feat(webapp,sdk,core): bulk delete endpoint for environment variables - #4948
matt-aitken wants to merge 8 commits into
Conversation
🦋 Changeset detectedLatest commit: c3847e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (31)
🧰 Additional context used📓 Path-based instructions (11)New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
Never use `request.signal` to detect client disconnects.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
Test files must not import `app/env.server.ts`; pass configuration as options instead.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md) Files:
Use zod for validation in packages/core and apps/webapp📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc) Files:
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` Use subpath exports from `@trigger.dev/core` package instead of importing from the root `@trigger.dev/core` path📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc) Files:
Use vitest for all tests in the Trigger.dev repository📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use function declarations instead of default exports📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
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 metr...📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc) Files:
🔇 Additional comments (1)
WalkthroughThe change adds bulk deletion for environment-variable values. It defines request and response schemas, an authenticated HTTP endpoint, core API client support, and SDK overloads. Repository deletion now supports batching, updater and parent-shadow filters, locking, version checks, secret cleanup, and removal of empty variables. Tests cover filtering, cleanup, project isolation, stale versions, duplicate keys, concurrency, and single-value deletion. Documentation and a changeset describe the endpoint and SDK method. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Concurrent environment-variable changes can cause an updated value or a newly recreated secret to be deleted. Resolve these deletion races before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed and relevant summary of the endpoint, filters, SDK support, concurrency behavior, and repository changes. It does not follow the repository template because it omits the issue reference, checklist, testing section, changelog section, and screenshots section.
✨ Finishing Touches 💡 1📝 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 |
Observability mapAs of over 461 measured of 479 entry points (base 27, no change) What this PR changed FIX FIRST
AUDIT 3 of 51 sensitive mutations record an actor. 48 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
POST /api/v1/projects/:projectRef/envvars/:slug/bulk-delete removes up to 1000 variables from one environment in a single call. The body can narrow the delete to values last written by a given source (onlyWrittenBy) or to branch values whose key also has a value on the parent environment (onlyShadowingParent); the response lists the keys deleted and the keys skipped. All value removal now goes through one helper that deletes the value rows, their secret references and secret store entries in a fixed number of statements for any number of keys, drops a variable left with no values, and skips a value that changed while the delete ran. The single-value delete uses the same path.
The shared value delete pads its per-row OR arms to a power of two so a call site does not mint one prepared statement per distinct row count, folds the emptied-variable sweep into one conditional delete, and takes the environment id per row so a caller cannot mix environments and clear the wrong secret store entry. Adds behavioural tests for the single-value delete path (last value drops the variable, other environments keep theirs, a value without a secret reference deletes cleanly) and for skipped duplicate and already-removed keys in the bulk delete.
Adds `envvars.bulkDelete(projectRef, slug, { keys, onlyWrittenBy?,
onlyShadowingParent? })`, resolving projectRef and slug from the task
context like the neighbouring envvars functions, plus the matching
`bulkDeleteEnvVars` API client method and param type. Documents the
endpoint in the management API reference and caps each key at 256
characters in the request schema.
fe628d7 to
79d2942
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Says in the API reference, the request schema and the client param type that a key is also skipped when its value changed while the delete ran, and that onlyShadowingParent only takes effect when the request addresses a preview branch. Adds a test for the padded row list path and matches the docs page title to its siblings.
The shared value delete locks the affected variable rows FOR UPDATE before sweeping variables left with no values, so a value inserted at the same moment is seen rather than cascaded away. With onlyShadowingParent the bulk delete also locks the parent values its candidates rely on and skips any key whose parent value is gone by the time the lock is granted, so a branch is never left without a value. Documents that a retry of an already committed request reports the keys it deleted as skipped, since the delete is idempotent.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 26d2a70e-4bc8-4532-81d6-eac86c31b6f5
📒 Files selected for processing (14)
.changeset/envvar-bulk-delete.mdapps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.tsapps/webapp/app/v3/environmentVariables/repository.tsapps/webapp/test/environmentVariablesRepository.test.tsapps/webapp/test/fixtures/environmentVariablesFixtures.tsdocs/docs.jsondocs/management/envvars/bulk-delete.mdxdocs/v3-openapi.yamlpackages/core/src/v3/apiClient/index.tspackages/core/src/v3/apiClient/types.tspackages/core/src/v3/schemas/api.tspackages/trigger-sdk/src/v3/envvars.tspackages/trigger-sdk/src/v3/index.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: internal / 🧪 Unit Tests: Internal (1)
- GitHub Check: internal / 🧪 Unit Tests: Internal (2)
⚠️ CI failures not shown inline (2)
GitHub Actions: 🛡️ E2E Tests: Webapp Auth (full) / 0_🛡️ E2E Auth Tests (full).txt: feat(webapp,sdk,core): bulk delete endpoint for environment variables
Conclusion: failure
ebapp/build/server/assets/server-build-D6iVC0qK.js:220342:18)\n at async ClickHouseRunsRepository.listRuns (file:///home/runner/work/trigger.dev/trigger.dev/apps/webapp/build/server/assets/server-build-D6iVC0qK.js:220386:36)\n at async file:///home/runner/work/trigger.dev/trigger.dev/apps/webapp/build/server/assets/server-build-D6iVC0qK.js:47527:14\n at async NextRunListPresenter.call (file:///home/runner/work/trigger.dev/trigger.dev/apps/webapp/build/server/assets/server-build-D6iVC0qK.js:292273:41)\n at async file:///home/runner/work/trigger.dev/trigger.dev/apps/webapp/build/server/assets/server-build-D6iVC0qK.js:480073:23"},"url":"http://localhost:42697/api/v1/runs","http":{"requestId":"U-IpdxJN90wpOHp6FCctx","path":"/api/v1/runs","host":"localhost","method":"GET","abortController":{}},"timestamp":"","name":"webapp","message":"Error in loader","level":"error"}
GET /api/v1/runs 500 - - 9.628 ms
[][ERROR][`@clickhouse/client`][Connection] Query: HTTP request error.
Arguments: {
query: 'SELECT run_id, toUnixTimestamp64Milli(created_at) AS created_at_ms FROM trigger_dev.task_runs_v2 FINAL WHERE organization_id = {organizationId: String} AND project_id = {projectId: String} AND environment_id = {environmentId: String} AND created_at >= fromUnixTimestamp64Milli({period: Int64}) ORDER BY created_at DESC, run_id DESC LIMIT 26 \n' +
'FORMAT JSONEachRow',
search_params: 'query_id=0767c8f7-6b27-4f46-9fb2-e1f81b375aa6¶m_organizationId=cmu5r35x9007b3x5j12ko1i86¶m_projectId=cmu5r35xa007d3x5j8j2q34ms¶m_environmentId=cmu5r35xc007f3x5j20n2vqhn¶m_period=1789058108359&max_execution_time=35&timeout_before_checking_execution_speed=0&max_threads=4&max_memory_usage=1073741824&readonly=2&output_format_json_quote_64bit_integers=0&output_format_json_quote_64bit_floats=0&cancel_http_readonly_queries_on_client_close=1',
with_abort_signal: false,
session_id: undefined,
query_id: '0767c8f7-6b27-4f46-9fb2-e1f81b375aa6',
decompress_response...
GitHub Actions: 🛡️ E2E Tests: Webapp Auth (full) / 🛡️ E2E Auth Tests (full): feat(webapp,sdk,core): bulk delete endpoint for environment variables
Conclusion: failure
ebapp/build/server/assets/server-build-D6iVC0qK.js:220342:18)\n at async ClickHouseRunsRepository.listRuns (file:///home/runner/work/trigger.dev/trigger.dev/apps/webapp/build/server/assets/server-build-D6iVC0qK.js:220386:36)\n at async file:///home/runner/work/trigger.dev/trigger.dev/apps/webapp/build/server/assets/server-build-D6iVC0qK.js:47527:14\n at async NextRunListPresenter.call (file:///home/runner/work/trigger.dev/trigger.dev/apps/webapp/build/server/assets/server-build-D6iVC0qK.js:292273:41)\n at async file:///home/runner/work/trigger.dev/trigger.dev/apps/webapp/build/server/assets/server-build-D6iVC0qK.js:480073:23"},"url":"http://localhost:42697/api/v1/runs","http":{"requestId":"U-IpdxJN90wpOHp6FCctx","path":"/api/v1/runs","host":"localhost","method":"GET","abortController":{}},"timestamp":"","name":"webapp","message":"Error in loader","level":"error"}
GET /api/v1/runs 500 - - 9.628 ms
[][ERROR][`@clickhouse/client`][Connection] Query: HTTP request error.
Arguments: {
query: 'SELECT run_id, toUnixTimestamp64Milli(created_at) AS created_at_ms FROM trigger_dev.task_runs_v2 FINAL WHERE organization_id = {organizationId: String} AND project_id = {projectId: String} AND environment_id = {environmentId: String} AND created_at >= fromUnixTimestamp64Milli({period: Int64}) ORDER BY created_at DESC, run_id DESC LIMIT 26 \n' +
'FORMAT JSONEachRow',
search_params: 'query_id=0767c8f7-6b27-4f46-9fb2-e1f81b375aa6¶m_organizationId=cmu5r35x9007b3x5j12ko1i86¶m_projectId=cmu5r35xa007d3x5j8j2q34ms¶m_environmentId=cmu5r35xc007f3x5j20n2vqhn¶m_period=1789058108359&max_execution_time=35&timeout_before_checking_execution_speed=0&max_threads=4&max_memory_usage=1073741824&readonly=2&output_format_json_quote_64bit_integers=0&output_format_json_quote_64bit_floats=0&cancel_http_readonly_queries_on_client_close=1',
with_abort_signal: false,
session_id: undefined,
query_id: '0767c8f7-6b27-4f46-9fb2-e1f81b375aa6',
decompress_response...
🧰 Additional context used
📓 Path-based instructions (18)
Use Remix flat-file route conventions with dot-separated segments; for example, `api.v1.tasks.$taskId.trigger.ts` maps to `/api/v1/tasks/:taskId/trigger`.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.ts
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/environmentVariables/repository.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
Never use `request.signal` to detect client disconnects.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/environmentVariables/repository.tsapps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/environmentVariablesRepository.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/environmentVariables/repository.tsapps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
**Public packages** (`packages/*`): Use `build`.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/core/src/v3/apiClient/index.tspackages/trigger-sdk/src/v3/envvars.tspackages/trigger-sdk/src/v3/index.tspackages/core/src/v3/apiClient/types.tspackages/core/src/v3/schemas/api.ts
Always import from `@trigger.dev/sdk`.
📄 CodeRabbit inference engine (packages/trigger-sdk/CLAUDE.md)
Files:
packages/trigger-sdk/src/v3/envvars.tspackages/trigger-sdk/src/v3/index.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/fixtures/environmentVariablesFixtures.tspackages/core/src/v3/apiClient/index.tsapps/webapp/app/v3/environmentVariables/repository.tspackages/core/src/v3/apiClient/types.tsapps/webapp/test/environmentVariablesRepository.test.tspackages/core/src/v3/schemas/api.tsapps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
In the Trigger.dev SDK (packages/trigger-sdk), prefer isomorphic code like fetch and ReadableStream instead of Node.js-specific code
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/trigger-sdk/src/v3/envvars.tspackages/trigger-sdk/src/v3/index.ts
Never import the root package (`@trigger.dev/core`).
📄 CodeRabbit inference engine (packages/core/CLAUDE.md)
Files:
packages/core/src/v3/apiClient/index.tspackages/core/src/v3/apiClient/types.tspackages/core/src/v3/schemas/api.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/environmentVariablesRepository.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` Use subpath exports from `@trigger.dev/core` package instead of importing from the root `@trigger.dev/core` path
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/fixtures/environmentVariablesFixtures.tsapps/webapp/app/v3/environmentVariables/repository.tsapps/webapp/test/environmentVariablesRepository.test.tsapps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/environmentVariablesRepository.test.ts
Main documentation config must be defined in `docs.json` which includes navigation structure, theme, and metadata Navigation structure in `docs.json` should be organized using `navigation.dropdowns` with groups and pages
📄 CodeRabbit inference engine (docs/CLAUDE.md)
Files:
docs/docs.json
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/fixtures/environmentVariablesFixtures.tspackages/core/src/v3/apiClient/index.tspackages/trigger-sdk/src/v3/envvars.tsapps/webapp/app/v3/environmentVariables/repository.tspackages/trigger-sdk/src/v3/index.tspackages/core/src/v3/apiClient/types.tsapps/webapp/test/environmentVariablesRepository.test.tspackages/core/src/v3/schemas/api.tsapps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
MDX documentation pages must include frontmatter with title (required), description (required), and sidebarTitle (optional) in YAML format Use Mintlify components for structured content: , , , , ,
📄 CodeRabbit inference engine (docs/CLAUDE.md)
Files:
docs/management/envvars/bulk-delete.mdx
docs/management/envvars/bulk-delete.mdxUse types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/fixtures/environmentVariablesFixtures.tspackages/core/src/v3/apiClient/index.tspackages/trigger-sdk/src/v3/envvars.tsapps/webapp/app/v3/environmentVariables/repository.tspackages/trigger-sdk/src/v3/index.tspackages/core/src/v3/apiClient/types.tsapps/webapp/test/environmentVariablesRepository.test.tspackages/core/src/v3/schemas/api.tsapps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.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 metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/test/fixtures/environmentVariablesFixtures.tspackages/core/src/v3/apiClient/index.tspackages/trigger-sdk/src/v3/envvars.tsapps/webapp/app/v3/environmentVariables/repository.tspackages/trigger-sdk/src/v3/index.tspackages/core/src/v3/apiClient/types.tsapps/webapp/test/environmentVariablesRepository.test.tspackages/core/src/v3/schemas/api.tsapps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
🧠 Learnings (2)
📚 Learning: 2026-06-14T17:36:56.078Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3942
File: docs/management/sessions/create.mdx:1-4
Timestamp: 2026-06-14T17:36:56.078Z
Learning: In trigger.dev docs, MDX pages under `docs/management/` that include an `openapi:` key in their frontmatter are intentional OpenAPI-driven reference stubs (typically `title` + `openapi` only). The page description is rendered from the referenced OpenAPI operation, so do not flag missing `description` frontmatter on these pages.
Applied to files:
docs/management/envvars/bulk-delete.mdx
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
packages/core/src/v3/apiClient/types.tsapps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
🪛 Checkov (3.3.16)
docs/v3-openapi.yaml
[high] 2-6679: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 2-6679: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🔇 Additional comments (15)
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (2)
1093-1096: The existing parent-shadow race still applies.The parent existence check remains unlocked before the branch value deletion. A concurrent parent deletion can still let this transaction delete the branch value based on an obsolete snapshot.
10-10: LGTM!Also applies to: 28-34, 64-151
apps/webapp/app/v3/environmentVariables/repository.ts (1)
9-9: LGTM!Also applies to: 66-76, 155-165
docs/v3-openapi.yaml (1)
2188-2305: LGTM!apps/webapp/test/fixtures/environmentVariablesFixtures.ts (1)
67-67: LGTM!Also applies to: 77-77
apps/webapp/test/environmentVariablesRepository.test.ts (1)
22-25: LGTM!Also applies to: 767-1095
docs/management/envvars/bulk-delete.mdx (1)
1-4: LGTM!.changeset/envvar-bulk-delete.md (1)
1-14: LGTM!packages/core/src/v3/schemas/api.ts (1)
1605-1610: LGTM!Also applies to: 1618-1618, 1625-1653
packages/core/src/v3/apiClient/types.ts (1)
2-2: LGTM!Also applies to: 33-45
apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.bulk-delete.ts (1)
1-91: LGTM!packages/core/src/v3/apiClient/index.ts (1)
56-56: LGTM!Also applies to: 133-133, 159-159, 1193-1210
packages/trigger-sdk/src/v3/envvars.ts (1)
4-5: LGTM!Also applies to: 20-24, 287-352
packages/trigger-sdk/src/v3/index.ts (1)
68-71: LGTM!docs/docs.json (1)
413-414: LGTM!
| await deleteEnvironmentVariableValueRows(tx, projectId, [ | ||
| { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '78,155p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
sed -n '940,1030p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
rg -n 'EnvironmentVariableValue.*version|version:.*increment|updateMany.*version|deleteEnvironmentVariableValueRows|deleteValue\(' apps/webapp/app/v3/environmentVariables apps/webapp/app -g '*.ts'Repository: triggerdotdev/trigger.dev
Length of output: 6684
🏁 Script executed:
rg -n -C 8 'environmentVariableValue\.(update|updateMany|create|upsert)|version\s*:\s*(\{|[A-Za-z_].*increment|.*version)|EnvironmentVariableValue' apps/webapp/app/v3/environmentVariables apps/webapp/app/routes packages apps/webapp/prisma prisma 2>/dev/null | head -n 260
rg -n -C 8 'function \$transaction|export .*\\$transaction|\\$transaction\s*=' apps/webapp/app/db.server.ts apps/webapp/app -g '*.ts' 2>/dev/null | head -n 160Repository: triggerdotdev/trigger.dev
Length of output: 32447
🏁 Script executed:
rg -n -C 25 'async editValue|editValue\(|environmentVariableValue:|EnvironmentVariableValue\s*\{' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts apps/webapp/prisma packages apps/webapp -g '*.prisma' -g '*.ts' | head -n 320
rg -n -C 12 'model EnvironmentVariableValue|version\s+Int' apps/webapp/prisma -g '*.prisma' | head -n 180Repository: triggerdotdev/trigger.dev
Length of output: 32335
🏁 Script executed:
sed -n '559,760p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
find apps/webapp -type f -name '*.prisma' -print
rg -n -C 12 'model EnvironmentVariableValue' apps/webapp --glob '*.prisma'Repository: triggerdotdev/trigger.dev
Length of output: 5817
Add the version guard to single-value deletion.
deleteValue reads the value before the transaction, while editValue can increment EnvironmentVariableValue.version in a concurrent transaction. Because this call omits version, the helper deletes by ID alone and can delete the newer value. The helper reports a version mismatch in skipped, but deleteValue ignores that result and reports success.
Proposed fix
select: {
id: true,
+ version: true,
valueReference: {
select: {
key: true,
@@
- await $transaction(this.prismaClient, "delete env var value", async (tx) => {
- await deleteEnvironmentVariableValueRows(tx, projectId, [
+ const deleted = await $transaction(this.prismaClient, "delete env var value", async (tx) => {
+ const result = await deleteEnvironmentVariableValueRows(tx, projectId, [
{
id: value.id,
+ version: value.version,
variableId: environmentVariable.id,
environmentId: options.environmentId,
key: environmentVariable.key,
secretReferenceKey: value.valueReference?.key,
},
]);
+
+ return result.deleted.length === 1;
});
+
+ if (deleted !== true) {
+ return {
+ success: false as const,
+ error: "Environment variable value changed before deletion",
+ };
+ }The bulk delete now locks the variable rows of every candidate before it touches any value row, and with onlyShadowingParent it does so before locking the parent values as well. That is the order a concurrent import takes through its variable upsert, so an import and a delete for the same variable can no longer wait on each other in a cycle and have one side aborted.
Editing a variable across several environments locked the value rows it updated before the insert of a new value took its implicit lock on the variable row, the opposite order to imports and deletes. It now locks the variable row as its first statement so every writer takes locks in the same order and none can wait on another in a cycle.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 982da3e1-f95b-4d7c-a9a8-b73f8d9d389d
📒 Files selected for processing (4)
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.tsapps/webapp/test/environmentVariablesRepository.test.tsdocs/v3-openapi.yamlpackages/core/src/v3/schemas/api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/v3/schemas/api.ts
- apps/webapp/test/environmentVariablesRepository.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (27)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: internal / 🧪 Unit Tests: Internal (2)
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: internal / 🧪 Unit Tests: Internal (1)
- GitHub Check: packages / 🧪 Unit Tests: Packages (2, 3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: 🛡️ E2E Auth Tests (full)
🧰 Additional context used
📓 Path-based instructions (8)
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
Never use `request.signal` to detect client disconnects.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` Use subpath exports from `@trigger.dev/core` package instead of importing from the root `@trigger.dev/core` path
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.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 metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
🪛 Checkov (3.3.16)
docs/v3-openapi.yaml
[high] 2-6679: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 2-6679: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
🔇 Additional comments (3)
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (2)
1036-1043: The single-value version guard is still missing.This is the same unresolved issue from the previous review.
deleteValuestill deletes by ID without passingvalue.version, so a concurrenteditValuecan update the value before this deletion removes it.
64-76: LGTM!Also applies to: 488-489, 1058-1136
docs/v3-openapi.yaml (1)
2251-2251: LGTM!
| await tx.$queryRaw<{ id: string }[]>` | ||
| SELECT "id" FROM "EnvironmentVariable" | ||
| WHERE "id" IN (${Prisma.join(boundedIn(variableIds))}) | ||
| FOR UPDATE | ||
| `; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '78,180p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
sed -n '430,540p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
sed -n '980,1175p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
rg -n 'secretKey\(|environmentVariable\.(create|upsert|delete)|lockEnvironmentVariableRows|isolationLevel|model EnvironmentVariable(Value)?' apps/webapp/app/v3/environmentVariables internal-packages/database apps/webapp/prisma --glob '*.ts' --glob '*.prisma'Repository: triggerdotdev/trigger.dev
Length of output: 15983
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- schema ---'
sed -n '2138,2195p' internal-packages/database/prisma/schema.prisma
printf '%s\n' '--- transaction helper ---'
sed -n '190,255p' internal-packages/database/src/transaction.ts
printf '%s\n' '--- create and nearby mutation paths ---'
sed -n '250,375p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
sed -n '600,675p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
printf '%s\n' '--- delete paths ---'
sed -n '900,985p' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
printf '%s\n' '--- all relevant lock and mutation references ---'
rg -n -C 4 'lockEnvironmentVariableRows|deleteEnvironmentVariableValueRows|environmentVariable\.(create|upsert|delete|deleteMany)|environmentVariableValue\.(create|upsert|delete|deleteMany)' apps/webapp/app/v3/environmentVariables apps/webapp/app --glob '*.ts'Repository: triggerdotdev/trigger.dev
Length of output: 40135
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- transaction wrapper definition ---'
rg -n -C 12 'export async function \$transaction|function \$transaction|isolationLevel' internal-packages/database/src apps/webapp/app/db.server.ts apps/webapp/app --glob '*.ts' | head -220
printf '%s\n' '--- repository transaction call sites ---'
rg -n -C 3 '\$transaction\(' apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
printf '%s\n' '--- transaction configuration references ---'
rg -n -C 5 'isolationLevel|TransactionIsolationLevel|Serializable|READ COMMITTED|ReadCommitted' apps/webapp internal-packages --glob '*.ts' --glob '*.json' --glob '*.prisma' | head -260Repository: triggerdotdev/trigger.dev
Length of output: 40962
Filter rows whose variable was not locked.
lockEnvironmentVariableRows discards the IDs returned by FOR UPDATE. Under the default transaction isolation, a prior candidate read can retain an old value after the parent is deleted. The foreign-key cascade removes that value, and the same (projectId, key) can then be recreated. The helper classifies the missing old value as deleted and removes the derived secret-store key, which can delete the recreated value's secret.
Return the locked variable IDs and filter rows before deleteMany and secret cleanup. Do not rely on the foreign-key cascade or the current mutation lock ordering to prevent this sequence; the whole-variable delete path deletes the parent without taking this lock.
Creating values for an existing variable and editing a single value did not lock the variable row before touching value rows, so a concurrent delete could still form a lock cycle with them. Both now lock the variable row up front, the same way edit and the delete paths do, and the lock helper's docblock is the single statement of that order. Also makes the concurrent parent-delete test deterministic by starting the bulk delete only after the competing delete has run.
Summary
Adds
POST /api/v1/projects/:projectRef/envvars/:slug/bulk-delete, which removes up to 1000 environment variables from one environment in a single call, plusenvvars.bulkDelete()in the SDK, a docs page and an OpenAPI entry.Two optional filters narrow the delete:
onlyWrittenBy: only remove values last written by the given source (a user, or an integration by name). Values set by anyone else are skipped.onlyShadowingParent: on a preview branch, only remove values whose key also has a value on the parent environment, so the branch falls back to the inherited value instead of ending up with nothing.The response lists the keys that were
deletedand the keys that wereskipped(no value in this environment, filtered out, or changed while the delete ran).The motivating case is an integration that syncs shared variables to a preview root and needs to retire the copies it previously wrote onto each branch without touching branch values a user set by hand. Doing that through the existing single DELETE endpoint would be one request per key with no way to express "only mine".
Design
All value removal in
EnvironmentVariablesRepositorynow goes through one helper. Given the already-selected value rows, it deletes them, theirSecretReferencerows and theirSecretStoreentries, then drops any variable left with no values, in a fixed number of statements regardless of how many keys were passed. EveryINlist and the per-row(id, version)arm list is padded to a power of two so the statement shape stays bounded. When a version is supplied the delete is guarded by it, so a value edited concurrently is reported as skipped rather than removed.The existing single-value
deleteValue(used by the dashboard and theDELETE .../envvars/:slug/:nameendpoint) was refactored onto the same helper, so there is one delete path rather than two. Two small behaviour changes fall out of that: it now returns success when the value row disappeared between its read and its delete, and it no longer fails when the secret store or reference row is already gone. Its variable lookup is also scoped to the caller's project.Two concurrency holes in that helper are closed with row locks inside the transaction, always taken in variable-then-value order to match
create. The variable rows are lockedFOR UPDATEfirst, so a concurrent value insert (which takesFOR KEY SHAREon the variable) either finishes before the emptiness check or waits until the delete commits and recreates the variable. WithonlyShadowingParent, the parent value rows are locked the same way and any key whose parent vanished meanwhile is skipped rather than leaving the branch with no value.create,editandeditValuetake the same variable lock before touching value rows, so every writer agrees on the order (the existing upsert increatetakes no lock when the variable already exists).The import endpoint is unchanged.