test: e2e auth baseline tests + webapp testcontainer infrastructure#3438
Open
matt-aitken wants to merge 3 commits intomainfrom
Open
test: e2e auth baseline tests + webapp testcontainer infrastructure#3438matt-aitken wants to merge 3 commits intomainfrom
matt-aitken wants to merge 3 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>
|
Contributor
WalkthroughAdds a new webapp E2E test suite that starts a shared test server, seeds a development test environment, generates and signs JWTs for tests, and verifies API and JWT bearer authentication and scope behavior. Introduces a Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
apps/webapp/test/api-auth.e2e.test.ts (2)
40-68: Consider seeding once per describe to cut e2e runtime.Each
itin this block callsseedTestEnvironment(server.prisma)even though three of them don't need a seeded environment at all (the "missing header", "invalid key", and "401 has error field" cases). Moving the seed into a sharedbeforeAll(or only calling it in the first test) avoids ~3 extra org/project/env inserts per run without affecting coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/test/api-auth.e2e.test.ts` around lines 40 - 68, Move the repeated call to seedTestEnvironment into a shared setup so we don't reseed for every it: inside the describe("API bearer auth — baseline behavior", ...) add a beforeAll that calls await seedTestEnvironment(server.prisma) and captures the returned apiKey in a scoped variable used by the first test; keep the remaining tests using no seed (or access the shared apiKey if needed) so only one seeding occurs per describe block and the tests "valid API key", "missing Authorization header", "invalid API key", and "401 response has error field" run against the single seeded environment.
108-120: Nit: this test may pass for the wrong reason.The JWT is signed with a random string but the payload still points at
environment.id. If, post‑RBAC refactor, the verifier path changes such that the token is silently accepted (e.g., signature check skipped in some branch) the assertionstatus === 401will still catch it — good. But since the intent is specifically "wrong signing key", consider also asserting the response body does not contain a successful shape, or add a positive counter‑test with the correct key to anchor the differential. Baseline-only — not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/webapp/test/api-auth.e2e.test.ts` around lines 108 - 120, The test "JWT signed with wrong key: 401" is ambiguous because it signs with a wrong secret but still uses environment.id; update the test to explicitly verify failure by checking the response body does not contain a successful payload (e.g., no runs, no auth token, or an error message) and also add a complementary positive test that signs a JWT with the correct environment signing key (use seedTestEnvironment to get the real key) and asserts a successful response; locate the test using generateJWT, seedTestEnvironment, server.webapp.fetch and update assertions accordingly to ensure the failure is due to signature mismatch rather than accidental acceptance.internal-packages/testcontainers/src/webapp.ts (3)
13-21:findFreePorthas an inherent TOCTOU window — acceptable but worth noting.Between
srv.close()and the webapp binding to the port, another process on CI could grab it. In practice this is rare on isolated CI workers, but if this suite becomes flaky withEADDRINUSE, consider retrying the spawn on bind failure or passingport: 0to the webapp directly and reading back the bound port from its logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/testcontainers/src/webapp.ts` around lines 13 - 21, The findFreePort function has a TOCTOU race (srv.close -> another process may bind) causing rare EADDRINUSE failures; update the startup flow to handle this by either retrying container/spawn on bind failure (catch EADDRINUSE and retry a few times) or avoid findFreePort entirely and start the webapp with port: 0, then read the actual bound port from the webapp process output/logs; look for references to findFreePort and the code that spawns the webapp to implement retry logic or switch to port: 0 + log-parsing to obtain the assigned port.
61-63: Hard-coded test secrets — fine for a harness, but make the intent clear.
SESSION_SECRET,MAGIC_LINK_SECRET,ENCRYPTION_KEYare static strings. That's correct for a throwaway e2e process, but please keep the// test-…prefix in the values (already done) and add a short comment that these are intentionally non-secret to prevent security scanners / reviewers from flagging them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/testcontainers/src/webapp.ts` around lines 61 - 63, The hard-coded secrets SESSION_SECRET, MAGIC_LINK_SECRET, and ENCRYPTION_KEY are fine for the test harness but need an explicit non-secret annotation to avoid scanner/reviewer flags: keep the existing "test-…" prefixes in the string values for SESSION_SECRET, MAGIC_LINK_SECRET, and ENCRYPTION_KEY and add a short inline comment such as "// intentionally non-secret for e2e tests" (or similar) on the same lines to clearly indicate these values are dummy/test-only.
23-33: Healthcheck loop silently swallows every error.The empty
catch {}is fine for the happy path (connection refused while the server boots), but when the webapp never becomes healthy the final error message is generic and the original cause (e.g., DNS, TLS, migration failure) is lost. Consider capturing the last error to include in the thrown message:♻️ Proposed fix
-async function waitForHealthcheck(url: string, timeoutMs = 60000): Promise<void> { - const deadline = Date.now() + timeoutMs; - while (Date.now() < deadline) { - try { - const res = await fetch(url); - if (res.ok) return; - } catch {} - await new Promise((r) => setTimeout(r, 500)); - } - throw new Error(`Webapp did not become healthy at ${url} within ${timeoutMs}ms`); -} +async function waitForHealthcheck(url: string, timeoutMs = 60_000): Promise<void> { + const deadline = Date.now() + timeoutMs; + let lastErr: unknown; + while (Date.now() < deadline) { + try { + const res = await fetch(url); + if (res.ok) return; + lastErr = new Error(`status ${res.status}`); + } catch (err) { + lastErr = err; + } + await new Promise((r) => setTimeout(r, 500)); + } + throw new Error( + `Webapp did not become healthy at ${url} within ${timeoutMs}ms (last error: ${String(lastErr)})` + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/testcontainers/src/webapp.ts` around lines 23 - 33, The healthcheck loop in waitForHealthcheck silently swallows errors and loses the root cause; modify waitForHealthcheck to capture the last thrown error (e.g., a local variable lastErr: unknown updated inside the catch block) and, when timing out, include that error's message/stack in the thrown Error thrown at the end (compose a descriptive message that contains the original lastErr information) so callers can see the underlying DNS/TLS/migration failure; keep the existing retry/sleep behavior and only change the catch to set lastErr = err.internal-packages/testcontainers/package.json (1)
7-10: Consider aligningmain/typeswith the exports map.
mainandtypesstill point at./src/index.tswhich is fine, but if you also want the./webappsubpath to be typed for TS consumers undermoduleResolution: node(non-Node16), you might eventually wanttypesVersionstoo. For now (pnpm workspace TS consumers withmoduleResolution: bundler/node16), this works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/testcontainers/package.json` around lines 7 - 10, The package.json exports map includes a "./webapp" subpath but the top-level "main" and "types" fields only point to "./src/index.ts", so TypeScript consumers using older moduleResolution modes won't get types for "./webapp"; update package.json to align exports and types by adding a types entry for the "./webapp" subpath (or add a "typesVersions" mapping) so that "./webapp" is typed for non-Node16 resolution; reference the "exports" object, the existing "main" and "types" fields, the "./webapp" subpath and add a corresponding types entry or a "typesVersions" configuration to map "./webapp" -> "./src/webapp.ts".
🤖 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/test/api-auth.e2e.test.ts`:
- Around line 9-15: The test file calls vi.setConfig(...) but does not import
vi, causing a ReferenceError; update the vitest import to include vi (e.g., add
vi to the named imports from "vitest") or remove the vi.setConfig(...) line and
rely on existing per-hook/test timeouts (edit the import line that currently
declares afterAll, beforeAll, describe, expect, it and the vi.setConfig call to
fix the mismatch).
In `@apps/webapp/test/helpers/seedTestEnvironment.ts`:
- Around line 3-10: The randomHex helper and its use in seedTestEnvironment rely
on Math.random which has limited entropy and can produce collisions for unique
fields (RuntimeEnvironment.apiKey, pkApiKey, Organization.slug, Project.slug,
Project.externalRef); replace randomHex with a crypto-backed generator (e.g.,
use crypto.randomBytes or crypto.randomUUID) to produce deterministic-length hex
strings or UUIDs, update calls in seedTestEnvironment to use that generator for
suffix, apiKey, and pkApiKey, and ensure any padding/truncation is done
predictably so uniqueness entropy is sufficient to avoid P2002 collisions.
In `@internal-packages/testcontainers/src/webapp.ts`:
- Around line 46-50: The code builds NODE_PATH using a hard-coded ":" separator
which breaks on Windows; update the logic in the webapp module to use
path.delimiter from Node's path module instead of ":"—import/require path
(symbol: path) and change the join in the existingNodePath/nodePath assignment
to use `${PNPM_HOISTED_MODULES}${path.delimiter}${existingNodePath}` (or
path.delimiter in the ternary branch), and make the same replacement for the
other occurrence that sets NODE_PATH around the nodePath symbol so the harness
works cross-platform.
- Around line 35-38: Replace the declared interfaces with type aliases: change
the exported interface named WebappInstance to an exported type alias
(preserving the shape: baseUrl: string and fetch(path: string, init?:
RequestInit): Promise<Response>) and do the same for the TestServer declaration;
ensure all references that previously relied on the interface name still work
(no behavioral changes, just syntactic replacement of interface → type for
WebappInstance and TestServer).
- Around line 131-146: startTestServer leaks resources when
createPostgresContainer succeeds but subsequent steps (new PrismaClient or
startWebapp) throw; wrap the setup sequence in a try/catch/finally so any
partially-initialized resources are torn down on failure. Specifically, in
startTestServer ensure you track created objects (network from new
Network().start(), container from createPostgresContainer, prisma from new
PrismaClient, and webapp/stopWebapp from startWebapp) and on error call the
appropriate cleanup for each initialized item (call stopWebapp() if created,
prisma.$disconnect() if created, container.stop(), and network.stop()) before
rethrowing the error. Ensure cleanup order is safe (stop webapp before
disconnecting prisma, and stop container/network last) and that you don’t
attempt to call methods on undefined resources.
- Around line 90-92: The proc.on("error", (err) => { ... }) handler in
startWebapp must not throw (which creates an uncaughtException); instead capture
the error and surface it through the module's readiness/stop flow—e.g., call the
readiness promise's reject handler or invoke the existing stop/cleanup callback
with the error so startWebapp's caller can observe failure; update the
proc.on("error") handler to call the readiness rejection function (or call the
exported stop/cleanup function) with the captured err rather than throwing, and
ensure any in-flight timers/listeners are cleaned up when you propagate the
error.
---
Nitpick comments:
In `@apps/webapp/test/api-auth.e2e.test.ts`:
- Around line 40-68: Move the repeated call to seedTestEnvironment into a shared
setup so we don't reseed for every it: inside the describe("API bearer auth —
baseline behavior", ...) add a beforeAll that calls await
seedTestEnvironment(server.prisma) and captures the returned apiKey in a scoped
variable used by the first test; keep the remaining tests using no seed (or
access the shared apiKey if needed) so only one seeding occurs per describe
block and the tests "valid API key", "missing Authorization header", "invalid
API key", and "401 response has error field" run against the single seeded
environment.
- Around line 108-120: The test "JWT signed with wrong key: 401" is ambiguous
because it signs with a wrong secret but still uses environment.id; update the
test to explicitly verify failure by checking the response body does not contain
a successful payload (e.g., no runs, no auth token, or an error message) and
also add a complementary positive test that signs a JWT with the correct
environment signing key (use seedTestEnvironment to get the real key) and
asserts a successful response; locate the test using generateJWT,
seedTestEnvironment, server.webapp.fetch and update assertions accordingly to
ensure the failure is due to signature mismatch rather than accidental
acceptance.
In `@internal-packages/testcontainers/package.json`:
- Around line 7-10: The package.json exports map includes a "./webapp" subpath
but the top-level "main" and "types" fields only point to "./src/index.ts", so
TypeScript consumers using older moduleResolution modes won't get types for
"./webapp"; update package.json to align exports and types by adding a types
entry for the "./webapp" subpath (or add a "typesVersions" mapping) so that
"./webapp" is typed for non-Node16 resolution; reference the "exports" object,
the existing "main" and "types" fields, the "./webapp" subpath and add a
corresponding types entry or a "typesVersions" configuration to map "./webapp"
-> "./src/webapp.ts".
In `@internal-packages/testcontainers/src/webapp.ts`:
- Around line 13-21: The findFreePort function has a TOCTOU race (srv.close ->
another process may bind) causing rare EADDRINUSE failures; update the startup
flow to handle this by either retrying container/spawn on bind failure (catch
EADDRINUSE and retry a few times) or avoid findFreePort entirely and start the
webapp with port: 0, then read the actual bound port from the webapp process
output/logs; look for references to findFreePort and the code that spawns the
webapp to implement retry logic or switch to port: 0 + log-parsing to obtain the
assigned port.
- Around line 61-63: The hard-coded secrets SESSION_SECRET, MAGIC_LINK_SECRET,
and ENCRYPTION_KEY are fine for the test harness but need an explicit non-secret
annotation to avoid scanner/reviewer flags: keep the existing "test-…" prefixes
in the string values for SESSION_SECRET, MAGIC_LINK_SECRET, and ENCRYPTION_KEY
and add a short inline comment such as "// intentionally non-secret for e2e
tests" (or similar) on the same lines to clearly indicate these values are
dummy/test-only.
- Around line 23-33: The healthcheck loop in waitForHealthcheck silently
swallows errors and loses the root cause; modify waitForHealthcheck to capture
the last thrown error (e.g., a local variable lastErr: unknown updated inside
the catch block) and, when timing out, include that error's message/stack in the
thrown Error thrown at the end (compose a descriptive message that contains the
original lastErr information) so callers can see the underlying
DNS/TLS/migration failure; keep the existing retry/sleep behavior and only
change the catch to set lastErr = err.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c8184d6-3e55-4bd8-a0e1-fcb4ffae6846
📒 Files selected for processing (5)
apps/webapp/test/api-auth.e2e.test.tsapps/webapp/test/helpers/seedTestEnvironment.tsinternal-packages/testcontainers/package.jsoninternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/webapp.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). (28)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 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 (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 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 (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
internal-packages/testcontainers/package.jsoninternal-packages/testcontainers/src/index.tsapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.ts
**/*.{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:
internal-packages/testcontainers/src/index.tsapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/index.tsapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/index.tsapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/index.tsapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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.tsapps/webapp/test/api-auth.e2e.test.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.tsapps/webapp/test/api-auth.e2e.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
apps/webapp/test/api-auth.e2e.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
Files:
apps/webapp/test/api-auth.e2e.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Use vitest exclusively for testing. Never mock anything — use testcontainers instead.
Place test files next to source files with naming patternMyService.ts->MyService.test.ts.
For Redis/PostgreSQL tests in vitest, use testcontainers helpers:redisTest,postgresTest, orcontainerTestimported from@internal/testcontainers.
Files:
apps/webapp/test/api-auth.e2e.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Do not import
env.server.tsdirectly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testableFor testable code, never import
env.server.tsin test files. Pass configuration as options instead (e.g.,realtimeClient.server.tstakes config as constructor arg,realtimeClientGlobal.server.tscreates singleton with env config)
Files:
apps/webapp/test/api-auth.e2e.test.ts
🧠 Learnings (24)
📓 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: 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
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`.
📚 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} : Use subpath exports from `trigger.dev/core` package instead of importing from the root `trigger.dev/core` path
Applied to files:
internal-packages/testcontainers/package.jsoninternal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-04-15T15:39:06.868Z
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`.
Applied to files:
internal-packages/testcontainers/package.jsoninternal-packages/testcontainers/src/index.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.ts
📚 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:
internal-packages/testcontainers/package.jsoninternal-packages/testcontainers/src/index.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-03-02T12:43:43.173Z
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
Applied to files:
internal-packages/testcontainers/package.jsoninternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-04-15T15:39:06.868Z
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.
Applied to files:
internal-packages/testcontainers/package.jsoninternal-packages/testcontainers/src/index.tsapps/webapp/test/api-auth.e2e.test.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 **/*.{ts,tsx,js,jsx} : Use function declarations instead of default exports
Applied to files:
internal-packages/testcontainers/package.json
📚 Learning: 2026-04-15T15:39:06.868Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-15T15:39:06.868Z
Learning: When modifying any public package (`packages/*` or `integrations/*`), add a changeset using `pnpm run changeset:add`.
Applied to files:
internal-packages/testcontainers/package.json
📚 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:
internal-packages/testcontainers/package.jsonapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/package.jsonapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.ts
📚 Learning: 2025-11-26T14:40:07.146Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 2710
File: packages/schema-to-json/package.json:0-0
Timestamp: 2025-11-26T14:40:07.146Z
Learning: Node.js 24+ has native TypeScript support and can execute .ts files directly without tsx or ts-node for scripts that use only erasable TypeScript syntax (type annotations, interfaces, etc.). The trigger.dev repository uses Node.js 24.11.1+ and scripts like updateVersion.ts can be run with `node` instead of `tsx`.
Applied to files:
internal-packages/testcontainers/package.json
📚 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:
internal-packages/testcontainers/package.json
📚 Learning: 2026-03-02T12:43:17.177Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/database/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:17.177Z
Learning: Applies to internal-packages/database/**/{app,src,webapp}/**/*.{ts,tsx,js,jsx} : Use `$replica` from `~/db.server` for read-heavy queries in the webapp instead of the primary database connection
Applied to files:
internal-packages/testcontainers/package.jsoninternal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-03-25T15:29:25.889Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2026-03-25T15:29:25.889Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Export every task, including subtasks
Applied to files:
internal-packages/testcontainers/package.json
📚 Learning: 2026-01-15T10:48:02.687Z
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
Applied to files:
internal-packages/testcontainers/src/index.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/index.tsapps/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:
internal-packages/testcontainers/src/index.tsapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/index.tsapps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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 **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repository
Applied to files:
apps/webapp/test/helpers/seedTestEnvironment.tsapps/webapp/test/api-auth.e2e.test.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.tsapps/webapp/test/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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-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-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/api-auth.e2e.test.tsinternal-packages/testcontainers/src/webapp.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: The webapp at apps/webapp is a Remix 2.1 application using Node.js v20
Applied to files:
internal-packages/testcontainers/src/webapp.ts
🔇 Additional comments (2)
internal-packages/testcontainers/src/index.ts (1)
21-21: LGTM — re-export looks fine.Exposing
createPostgresContaineralongsideassertNonNullablematches the newstartTestServer()consumer inwebapp.ts.internal-packages/testcontainers/src/webapp.ts (1)
52-70: All truly unconditional required env vars are present; setup is correct for auth tests only.The env.server.ts declares 11 strictly required vars (no defaults), but most are conditionally required:
GITHUB_APP_ID,GITHUB_APP_PRIVATE_KEY,GITHUB_APP_WEBHOOK_SECRET,GITHUB_APP_SLUG— only required whenGITHUB_APP_ENABLED="1"(defaults to"0"via preprocessor)S2_ACCESS_TOKEN,S2_DEPLOYMENT_LOGS_BASIN_NAME— only required whenS2_ENABLED="1"(defaults to"0"via preprocessor)SESSION_SECRET,MAGIC_LINK_SECRET,ENCRYPTION_KEY,DEPLOY_REGISTRY_HOST,CLICKHOUSE_URL— all present in the harnessThe testcontainer is scoped to
api-auth.e2e.test.ts(auth baseline tests), which only exercises JWT and API key validation paths. Those routes don't touch GitHub integration, S2 deployment logs, queues, realtime, or object store — all correctly disabled. If you later expand tests to cover features requiring those subsystems, you'll need to add the corresponding env vars to the harness.
Comment on lines
+9
to
+15
| import { afterAll, beforeAll, describe, expect, it } from "vitest"; | ||
| import type { TestServer } from "@internal/testcontainers/webapp"; | ||
| import { startTestServer } from "@internal/testcontainers/webapp"; | ||
| import { generateJWT } from "@trigger.dev/core/v3/jwt"; | ||
| import { seedTestEnvironment } from "./helpers/seedTestEnvironment"; | ||
|
|
||
| vi.setConfig({ testTimeout: 180_000 }); |
Contributor
There was a problem hiding this comment.
vi is used but never imported — this file will throw ReferenceError at load time.
Line 15 calls vi.setConfig(...), but vi is not in the vitest import on line 9. Unless globals: true is enabled for vitest in this workspace, this will crash before any test runs. The PR description also notes "CI not yet passing" — this is almost certainly one of the reasons.
🐛 Proposed fix
-import { afterAll, beforeAll, describe, expect, it } from "vitest";
+import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";Alternatively, drop the vi.setConfig call and rely on the per-hook timeouts already provided to beforeAll/afterAll plus a per-it timeout (or set testTimeout in vitest.config.ts).
📝 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.
Suggested change
| import { afterAll, beforeAll, describe, expect, it } from "vitest"; | |
| import type { TestServer } from "@internal/testcontainers/webapp"; | |
| import { startTestServer } from "@internal/testcontainers/webapp"; | |
| import { generateJWT } from "@trigger.dev/core/v3/jwt"; | |
| import { seedTestEnvironment } from "./helpers/seedTestEnvironment"; | |
| vi.setConfig({ testTimeout: 180_000 }); | |
| import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; | |
| import type { TestServer } from "@internal/testcontainers/webapp"; | |
| import { startTestServer } from "@internal/testcontainers/webapp"; | |
| import { generateJWT } from "@trigger.dev/core/v3/jwt"; | |
| import { seedTestEnvironment } from "./helpers/seedTestEnvironment"; | |
| vi.setConfig({ testTimeout: 180_000 }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/test/api-auth.e2e.test.ts` around lines 9 - 15, The test file
calls vi.setConfig(...) but does not import vi, causing a ReferenceError; update
the vitest import to include vi (e.g., add vi to the named imports from
"vitest") or remove the vi.setConfig(...) line and rely on existing
per-hook/test timeouts (edit the import line that currently declares afterAll,
beforeAll, describe, expect, it and the vi.setConfig call to fix the mismatch).
Comment on lines
+35
to
+38
| export interface WebappInstance { | ||
| baseUrl: string; | ||
| fetch(path: string, init?: RequestInit): Promise<Response>; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use type aliases instead of interface per coding guidelines.
Both WebappInstance and TestServer are declared as interface.
♻️ Proposed fix
-export interface WebappInstance {
- baseUrl: string;
- fetch(path: string, init?: RequestInit): Promise<Response>;
-}
+export type WebappInstance = {
+ baseUrl: string;
+ fetch(path: string, init?: RequestInit): Promise<Response>;
+};
@@
-export interface TestServer {
- webapp: WebappInstance;
- prisma: PrismaClient;
- stop: () => Promise<void>;
-}
+export type TestServer = {
+ webapp: WebappInstance;
+ prisma: PrismaClient;
+ stop: () => Promise<void>;
+};As per coding guidelines: "Use types over interfaces for TypeScript".
Also applies to: 124-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal-packages/testcontainers/src/webapp.ts` around lines 35 - 38, Replace
the declared interfaces with type aliases: change the exported interface named
WebappInstance to an exported type alias (preserving the shape: baseUrl: string
and fetch(path: string, init?: RequestInit): Promise<Response>) and do the same
for the TestServer declaration; ensure all references that previously relied on
the interface name still work (no behavioral changes, just syntactic replacement
of interface → type for WebappInstance and TestServer).
Comment on lines
+46
to
+50
| // Merge NODE_PATH so transitive pnpm deps (hoisted to .pnpm/node_modules) are resolvable | ||
| const existingNodePath = process.env.NODE_PATH; | ||
| const nodePath = existingNodePath | ||
| ? `${PNPM_HOISTED_MODULES}:${existingNodePath}` | ||
| : PNPM_HOISTED_MODULES; |
Contributor
There was a problem hiding this comment.
NODE_PATH uses a hard-coded : separator — break on Windows.
NODE_PATH entries are joined with a platform-dependent separator. Use path.delimiter instead of ":" so this harness works on Windows developer machines as well (CI is Linux, but contributors aren't always).
🛠️ Proposed fix
-import { resolve } from "path";
+import { delimiter, resolve } from "path";
@@
- const nodePath = existingNodePath
- ? `${PNPM_HOISTED_MODULES}:${existingNodePath}`
- : PNPM_HOISTED_MODULES;
+ const nodePath = existingNodePath
+ ? `${PNPM_HOISTED_MODULES}${delimiter}${existingNodePath}`
+ : PNPM_HOISTED_MODULES;Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal-packages/testcontainers/src/webapp.ts` around lines 46 - 50, The
code builds NODE_PATH using a hard-coded ":" separator which breaks on Windows;
update the logic in the webapp module to use path.delimiter from Node's path
module instead of ":"—import/require path (symbol: path) and change the join in
the existingNodePath/nodePath assignment to use
`${PNPM_HOISTED_MODULES}${path.delimiter}${existingNodePath}` (or path.delimiter
in the ternary branch), and make the same replacement for the other occurrence
that sets NODE_PATH around the nodePath symbol so the harness works
cross-platform.
![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.
Contributor
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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal-packages/testcontainers/src/webapp.ts (2)
35-38: 🛠️ Refactor suggestion | 🟠 MajorUse
typealiases instead ofinterface(coding guideline).
WebappInstance(lines 35-38) andTestServer(lines 130-134) should be declared astypealiases per the repo's TypeScript guideline.♻️ Proposed fix
-export interface WebappInstance { - baseUrl: string; - fetch(path: string, init?: RequestInit): Promise<Response>; -} +export type WebappInstance = { + baseUrl: string; + fetch(path: string, init?: RequestInit): Promise<Response>; +}; @@ -export interface TestServer { - webapp: WebappInstance; - prisma: PrismaClient; - stop: () => Promise<void>; -} +export type TestServer = { + webapp: WebappInstance; + prisma: PrismaClient; + stop: () => Promise<void>; +};As per coding guidelines: "Use types over interfaces for TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/testcontainers/src/webapp.ts` around lines 35 - 38, Replace the declared interfaces with type aliases: change the WebappInstance and TestServer declarations from "interface X { ... }" to "type X = { ... }" (use the same property signatures), ensuring you export them the same way and keep method signatures (e.g., fetch(path: string, init?: RequestInit): Promise<Response>) intact; update any local references if necessary to match the type alias names.
46-50:⚠️ Potential issue | 🟡 Minor
NODE_PATHuses a hard-coded:separator — breaks on Windows.Use
path.delimiterso this harness works for contributors on Windows (CI is Linux, but local dev environments vary).🛠️ Proposed fix
-import { resolve } from "path"; +import { delimiter, resolve } from "path"; @@ - const nodePath = existingNodePath - ? `${PNPM_HOISTED_MODULES}:${existingNodePath}` - : PNPM_HOISTED_MODULES; + const nodePath = existingNodePath + ? `${PNPM_HOISTED_MODULES}${delimiter}${existingNodePath}` + : PNPM_HOISTED_MODULES;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/testcontainers/src/webapp.ts` around lines 46 - 50, The code builds NODE_PATH using a hard-coded ':' separator which breaks on Windows; update the logic that computes nodePath (variables: existingNodePath, nodePath, PNPM_HOISTED_MODULES, process.env.NODE_PATH) to join PNPM_HOISTED_MODULES and existingNodePath using path.delimiter instead of ':' and ensure the module 'path' is imported/required at the top of the file so the correct OS-specific separator is used.
🧹 Nitpick comments (2)
internal-packages/testcontainers/src/webapp.ts (2)
100-108: Redundant pre-healthcheckspawnErrorcheck.The
if (spawnError) throw spawnError;at line 101 runs synchronously right afterspawn()returns, before any'error'event can fire (those events are emitted asynchronously viaprocess.nextTick/microtask). It will never be true here; only the check at line 103 (after awaiting the healthcheck) is meaningful. You can drop line 101 without changing behavior.♻️ Proposed fix
try { - if (spawnError) throw spawnError; await waitForHealthcheck(`${baseUrl}/healthcheck`); if (spawnError) throw spawnError;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/testcontainers/src/webapp.ts` around lines 100 - 108, Remove the redundant synchronous pre-healthcheck guard that checks spawnError immediately after spawn(): delete the first "if (spawnError) throw spawnError;" in the try block (the one before await waitForHealthcheck) so only the meaningful post-healthcheck check remains; this targets the try block in webapp startup logic (the code around waitForHealthcheck and proc.kill) and leaves the subsequent "if (spawnError) throw spawnError;" after the await intact.
8-11: Fragile relative path toapps/webapp.
WEBAPP_ROOTandPNPM_HOISTED_MODULESare computed via fixed../../../hops from__dirname. This breaks silently if the file moves or if__dirnameresolves to a built output directory (e.g.,dist/) rather thansrc/. Consider resolving from a more stable anchor (e.g., walking up to the nearestpnpm-workspace.yaml) or at least asserting the resolved paths exist at startup with a clear error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal-packages/testcontainers/src/webapp.ts` around lines 8 - 11, The hardcoded relative resolution for WEBAPP_ROOT and PNPM_HOISTED_MODULES is fragile; update webapp.ts to locate a stable anchor instead of relying on "../../../" hops: implement a small upward walk from __dirname to find the nearest pnpm-workspace.yaml (or another repo root marker) and use that directory as the base to compute WEBAPP_ROOT and PNPM_HOISTED_MODULES, and if that marker or the resolved paths do not exist, throw/startup log a clear error explaining the missing path; ensure references to the constants WEBAPP_ROOT and PNPM_HOISTED_MODULES are updated to use the computed stable base so the code works whether running from src/ or dist/.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal-packages/testcontainers/src/webapp.ts`:
- Around line 163-168: The stop() helper aborts teardown on the first thrown
error and can leak resources; update stop() so each teardown step (stopWebapp(),
prisma!.$disconnect(), container!.stop(), network.stop()) is invoked in its own
try/catch (or otherwise awaited with failure swallowed and logged) to mirror the
best-effort pattern used earlier, ensuring that failures in stopWebapp or prisma
disconnect do not prevent container!.stop() and network.stop() from running and
that errors are logged rather than rethrown.
---
Duplicate comments:
In `@internal-packages/testcontainers/src/webapp.ts`:
- Around line 35-38: Replace the declared interfaces with type aliases: change
the WebappInstance and TestServer declarations from "interface X { ... }" to
"type X = { ... }" (use the same property signatures), ensuring you export them
the same way and keep method signatures (e.g., fetch(path: string, init?:
RequestInit): Promise<Response>) intact; update any local references if
necessary to match the type alias names.
- Around line 46-50: The code builds NODE_PATH using a hard-coded ':' separator
which breaks on Windows; update the logic that computes nodePath (variables:
existingNodePath, nodePath, PNPM_HOISTED_MODULES, process.env.NODE_PATH) to join
PNPM_HOISTED_MODULES and existingNodePath using path.delimiter instead of ':'
and ensure the module 'path' is imported/required at the top of the file so the
correct OS-specific separator is used.
---
Nitpick comments:
In `@internal-packages/testcontainers/src/webapp.ts`:
- Around line 100-108: Remove the redundant synchronous pre-healthcheck guard
that checks spawnError immediately after spawn(): delete the first "if
(spawnError) throw spawnError;" in the try block (the one before await
waitForHealthcheck) so only the meaningful post-healthcheck check remains; this
targets the try block in webapp startup logic (the code around
waitForHealthcheck and proc.kill) and leaves the subsequent "if (spawnError)
throw spawnError;" after the await intact.
- Around line 8-11: The hardcoded relative resolution for WEBAPP_ROOT and
PNPM_HOISTED_MODULES is fragile; update webapp.ts to locate a stable anchor
instead of relying on "../../../" hops: implement a small upward walk from
__dirname to find the nearest pnpm-workspace.yaml (or another repo root marker)
and use that directory as the base to compute WEBAPP_ROOT and
PNPM_HOISTED_MODULES, and if that marker or the resolved paths do not exist,
throw/startup log a clear error explaining the missing path; ensure references
to the constants WEBAPP_ROOT and PNPM_HOISTED_MODULES are updated to use the
computed stable base so the code works whether running from src/ or dist/.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4878a9af-f870-467c-b7c9-5f7a7e50e2e7
📒 Files selected for processing (1)
internal-packages/testcontainers/src/webapp.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). (28)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.ts
🧠 Learnings (23)
📓 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: 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: 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: 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: 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: 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-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:
internal-packages/testcontainers/src/webapp.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/**/*.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:
internal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-03-02T12:43:17.177Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/database/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:17.177Z
Learning: Applies to internal-packages/database/**/{app,src,webapp}/**/*.{ts,tsx,js,jsx} : Use `$replica` from `~/db.server` for read-heavy queries in the webapp instead of the primary database connection
Applied to files:
internal-packages/testcontainers/src/webapp.ts
📚 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: All new run lifecycle logic should be implemented in the `internal/run-engine` package (`src/engine/`), not directly in `apps/webapp/app/v3/services/`
Applied to files:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-04-15T15:39:06.868Z
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`.
Applied to files:
internal-packages/testcontainers/src/webapp.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: The webapp at apps/webapp is a Remix 2.1 application using Node.js v20
Applied to files:
internal-packages/testcontainers/src/webapp.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} : Use subpath exports from `trigger.dev/core` package instead of importing from the root `trigger.dev/core` path
Applied to files:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.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 **/*.{ts,tsx} : Use types over interfaces for TypeScript
Applied to files:
internal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-01-15T10:48:02.687Z
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
Applied to files:
internal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-04-15T15:39:06.868Z
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.
Applied to files:
internal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-03-02T12:43:43.173Z
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
Applied to files:
internal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-04-07T14:12:59.018Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3331
File: apps/webapp/app/runEngine/concerns/batchPayloads.server.ts:112-136
Timestamp: 2026-04-07T14:12:59.018Z
Learning: In `apps/webapp/app/runEngine/concerns/batchPayloads.server.ts`, the `pRetry` call wrapping `uploadPacketToObjectStore` intentionally retries **all** error types (no `shouldRetry` filter / `AbortError` guards). The maintainer explicitly prefers over-retrying to under-retrying because multiple heterogeneous object store backends are supported and it is impractical to enumerate all permanent error signatures. Do not flag this as an issue in future reviews.
Applied to files:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-03-26T23:24:54.682Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3114
File: apps/supervisor/src/workloadServer/index.ts:817-825
Timestamp: 2026-03-26T23:24:54.682Z
Learning: In `apps/supervisor/src/workloadServer/index.ts` (`WorkloadServer.stop()`), pending items returned by `this.snapshotDelayWheel?.stop()` are intentionally logged and dropped rather than dispatched. The entire supervisor is shutting down, so the snapshot callback URL would point at a dead server; dispatching snapshots during teardown would create orphaned gateway callbacks. Runners detect the supervisor is gone and reconnect to a new supervisor instance, which re-triggers the snapshot workflow. Do not flag the drop-on-shutdown behavior as a bug.
Applied to files:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.ts
📚 Learning: 2026-04-16T14:07:46.808Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3399
File: apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts:282-291
Timestamp: 2026-04-16T14:07:46.808Z
Learning: In `apps/webapp/app/services/realtime/redisRealtimeStreams.server.ts` (`streamResponse`), the pattern `signal.addEventListener("abort", cleanup, { once: true })` does NOT need an explicit `removeEventListener` call in the non-abort cleanup paths (inactivity, cancel). The `AbortController` is per-request, scoped to `httpAsyncStorage` (created in `apps/webapp/server.ts` per-request middleware), so it gets GC'd when the request ends — taking the listener and closure with it. The `isCleanedUp` guard prevents double-execution, and `redis.disconnect()` is called before the request ends. Do not flag this as a listener/closure leak.
Applied to files:
internal-packages/testcontainers/src/webapp.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:
internal-packages/testcontainers/src/webapp.ts
Comment on lines
+163
to
+168
| const stop = async () => { | ||
| await stopWebapp!(); | ||
| await prisma!.$disconnect(); | ||
| await container!.stop(); | ||
| await network.stop(); | ||
| }; |
Contributor
There was a problem hiding this comment.
stop() aborts teardown on first failure, leaking remaining resources.
If stopWebapp() or prisma.$disconnect() rejects, the container and network never stop, leaking across test files on CI. Mirror the best-effort pattern used in the catch block at lines 156-159.
♻️ Proposed fix
const stop = async () => {
- await stopWebapp!();
- await prisma!.$disconnect();
- await container!.stop();
- await network.stop();
+ await stopWebapp!().catch(() => {});
+ await prisma!.$disconnect().catch(() => {});
+ await container!.stop().catch(() => {});
+ await network.stop().catch(() => {});
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal-packages/testcontainers/src/webapp.ts` around lines 163 - 168, The
stop() helper aborts teardown on the first thrown error and can leak resources;
update stop() so each teardown step (stopWebapp(), prisma!.$disconnect(),
container!.stop(), network.stop()) is invoked in its own try/catch (or otherwise
awaited with failure swallowed and logged) to mirror the best-effort pattern
used earlier, ensuring that failures in stopWebapp or prisma disconnect do not
prevent container!.stop() and network.stop() from running and that errors are
logged rather than rethrown.
Comment on lines
+163
to
+168
| const stop = async () => { | ||
| await stopWebapp!(); | ||
| await prisma!.$disconnect(); | ||
| await container!.stop(); | ||
| await network.stop(); | ||
| }; |
Contributor
There was a problem hiding this comment.
🟡 Happy-path stop() lacks per-step error handling, can leak Docker resources
The stop function returned by startTestServer() (lines 163-168) doesn't guard individual cleanup steps with .catch(), unlike the error-path cleanup (lines 155-160). If any step (e.g. container!.stop()) throws, all subsequent steps (network.stop()) are skipped — leaking Docker networks and potentially containers. This is inconsistent with the error-path pattern at internal-packages/testcontainers/src/webapp.ts:155-160 which correctly uses .catch(() => {}) on each step. In a test suite that runs many test files, leaked Docker networks can accumulate.
Suggested change
| const stop = async () => { | |
| await stopWebapp!(); | |
| await prisma!.$disconnect(); | |
| await container!.stop(); | |
| await network.stop(); | |
| }; | |
| const stop = async () => { | |
| await stopWebapp!().catch(() => {}); | |
| await prisma!.$disconnect().catch(() => {}); | |
| await container!.stop().catch(() => {}); | |
| await network.stop().catch(() => {}); | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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