-
-
Notifications
You must be signed in to change notification settings - Fork 687
Set env vars to be "secret" #1923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
WalkthroughThis set of changes introduces the concept of "secret" environment variables across the application stack. The database schema is updated to include an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant Backend
participant Database
User->>WebApp: Mark env var as "Secret" in UI
WebApp->>Backend: Create env var with isSecret=true
Backend->>Database: Insert env var value with isSecret=true
Database-->>Backend: Confirmation
Backend-->>WebApp: Success
User->>WebApp: View environment variables
WebApp->>Backend: Request env vars (with redacted secrets)
Backend->>Database: Query env vars, redact secrets
Database-->>Backend: Env var values (+isSecret)
Backend-->>WebApp: Env vars (secrets redacted, isSecret flag)
WebApp-->>User: Display, show lock icon/tooltips for secrets
User->>WebApp: Retrieve single env var via API
WebApp->>Backend: API call for env var (with redacted secrets)
Backend->>Database: Query env var, redact if secret
Database-->>Backend: Env var value (+isSecret)
Backend-->>WebApp: Env var (secret redacted if needed)
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
* update nypm to support text-based bun lockfiles * add nypm changeset * handle dev flushing failures gracefully * fix path normalization for init.ts * add changesets * chore: remove pre.json after exiting pre mode * init command to install v4-beta packages * Revert "chore: remove pre.json after exiting pre mode" This reverts commit f5694fd. * make init default to cli version for all packages
* chore: Update version for release (v4-beta) * Release 4.0.0-v4-beta.1 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: nicktrn <[email protected]>
* run engine v1 will only lock to v1 deployments * run engine v2 will only lock to managed v2 deployments * test: create background worker and deployment with correct engine version
* link from deployments tasks to filtered runs view * jump to deployment * don't add version links for dev (yet)
* only return last v1 deployment in the shared queue consumer * be explicit about only returning managed deployments
* Add a docs page for the human-in-the-loop example project * Order guides, example projects and example tasks alphabetically in the docs list
* update nypm to support text-based bun lockfiles * fix retry spans * only download debug logs if admin * add nypm changeset * pull out env override logic * use runner env gather helper * handle dev flushing failures gracefully * fix path normalization for init.ts * add logger * add execution heartbeat service * add snapshot poller service * fix poller * add changesets * create socket in constructor * enable strictPropertyInitialization * deprecate dequeue from version * start is not async * dependency injection in prep for tests * add warm start count to all controller logs * add restore count * pull out run execution logic * temp disable pre * add a controller log when starting an execution * refactor execution and squash some bugs * cleanup completed docker containers by default * execution fixes and logging improvements * don't throw afet abort cleanup * poller should use private interval * rename heartbeat service file * rename HeartbeatService to IntervalService * restore old heartbeat service but deprecate it * use the new interval service everywhere * Revert "temp disable pre" This reverts commit e03f417. * add changeset * replace all run engine find uniques with find first
* chore: Update version for release (v4-beta) * Release 4.0.0-v4-beta.2 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: nicktrn <[email protected]>
* add failing test case * do not carry over previous batch id when blocking with waitpoint * delete irrelevant test
* Delete project - Don’t schedule tasks if the project is deleted - Delete queues from the master queues - Add the old delete project UI back in * Mark the project as deleted last * Fix for overriding local variable * Added a todo for deleting env queues * Remove todo
* add flush to global usage api * enable controller debug logs * initialize usage manager after env overrides * add previous run id to more debug logs * add changeset
# Conflicts: # .changeset/pre.json # packages/build/CHANGELOG.md # packages/build/package.json # packages/cli-v3/CHANGELOG.md # packages/cli-v3/package.json # packages/cli-v3/src/entryPoints/managed/env.ts # packages/cli-v3/src/entryPoints/managed/execution.ts # packages/core/CHANGELOG.md # packages/core/package.json # packages/python/CHANGELOG.md # packages/python/package.json # packages/react-hooks/CHANGELOG.md # packages/react-hooks/package.json # packages/redis-worker/CHANGELOG.md # packages/redis-worker/package.json # packages/rsc/CHANGELOG.md # packages/rsc/package.json # packages/trigger-sdk/CHANGELOG.md # packages/trigger-sdk/package.json # pnpm-lock.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx (2)
404-412
:⚠️ Potential issueDuplicate HTML
id
attributes across multiple edit dialogs
useForm
is initialised with a static string ("edit-environment-variable"
). Because an edit panel is rendered for every table row (even when closed), this creates duplicateid
attributes in the DOM – violating HTML spec and breaking label↔input associations for assistive tech.Generate a stable, per‑variable id instead:
- const [form, { id, environmentId, value }] = useForm({ - id: "edit-environment-variable", + const [form, { id, environmentId, value }] = useForm({ + id: `edit-env-var-${variable.id}`,Do the same for the delete form below.
487-495
:⚠️ Potential issueSame duplicate‑ID issue for delete forms
Every row gets an identical form id (
"delete-environment-variable"
). Use the variable id to keep them unique:- const [form, { id }] = useForm({ - id: "delete-environment-variable", + const [form, { id }] = useForm({ + id: `delete-env-var-${variable.id}`,
🧹 Nitpick comments (11)
apps/webapp/app/components/primitives/Switch.tsx (1)
36-42
: Add documentation for the new "medium" variant.
Themedium
variant styling integrates well between existingsmall
andlarge
variants. Please update any relevant component documentation or Storybook entries to include this variant and ensure it’s covered by your UI tests.internal-packages/database/prisma/migrations/20250411172850_environment_variable_value_is_secret/migration.sql (1)
1-3
: AddisSecret
column toEnvironmentVariableValue
.
This migration correctly adds a non-nullableisSecret
flag defaulting tofalse
. Consider adding an index on this column if you anticipate filtering environment variable values by secret status frequently to improve query performance.apps/webapp/app/presenters/v3/EnvironmentVariablesPresenter.server.ts (3)
1-1
: Remove unused import to avoid dead code and linter noise
flipCauseOption
is imported but never referenced inside this module. Keeping unused imports can hide real dead‑code issues and may cause linter/CI failures.-import { flipCauseOption } from "effect/Cause";
87-90
: Move the “who‑can‑see‑this‑environment” filter into the DB queryFiltering with
Array.filter
after fetching all environments works, but it:
- Retrieves rows the user is not allowed to see, then discards them.
- Prevents the database from using an index to enforce access constraints.
Consider pushing the
orgMember?.userId === userId
clause into theruntimeEnvironment.findMany
where
condition so the DB only returns rows the caller is authorised to access.
This saves bandwidth and protects against future callers accidentally using the unfiltered list.
96-118
: Pre‑index variables by key to avoid an O(N²) nested lookupInside the double
flatMap
,variables.find((v) => v.key === environmentVariable.key)
runs on every iteration, producing quadratic behaviour when many variables exist.-const variables = await repository.getProject(project.id); +const variables = await repository.getProject(project.id); +const variablesByKey = new Map(variables.map((v) => [v.key, v])); -const variable = variables.find((v) => v.key === environmentVariable.key); +const variable = variablesByKey.get(environmentVariable.key);For hundreds of env‑vars this removes a hot‑path linear search and keeps the presenter responsive.
references/hello-world/src/trigger/envvars.ts (2)
18-22
: Harden the test by asserting the variable exists before dereferencingIf the fixture ever contains only secret variables,
nonSecretEnvVar
becomesundefined
, causing an unhelpful assertion failure on the next line.-const nonSecretEnvVar = vars.find((v) => !v.isSecret); -assert.equal(nonSecretEnvVar?.isSecret, false); +const nonSecretEnvVar = vars.find((v) => !v.isSecret); +assert.ok(nonSecretEnvVar, "No non‑secret env var found in fixture"); +assert.equal(nonSecretEnvVar.isSecret, false);Do the same for the secret case below to surface clearer diagnostics.
33-36
: Mirror the “exists” check for the secret variable-const secretEnvVar = vars.find((v) => v.isSecret); -assert.equal(secretEnvVar?.isSecret, true); +const secretEnvVar = vars.find((v) => v.isSecret); +assert.ok(secretEnvVar, "No secret env var found in fixture"); +assert.equal(secretEnvVar.isSecret, true);packages/cli-v3/src/entryPoints/managed/heartbeat.ts (1)
81-83
: Clarify units inupdateInterval
or convert seconds→ms consistently
updateInterval(intervalMs: number)
accepts milliseconds, but callers might pass seconds by analogy with the constructor. Consider either:
- Renaming the parameter to
intervalMs
in the public API, or- Accepting seconds and converting internally, mirroring the constructor.
This avoids accidental 1000× interval errors.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx (3)
230-236
: Consistent spelling & DX: renameoccurences
→occurrences
The property is misspelled when attached to each mapped variable (
occurences
). Besides the readability hit, this propagates into every consumer, increasing the chance of subtle bugs.- occurences: totalOccurrences, + occurrences: totalOccurrences,Remember to update all downstream references (
variable.occurences
in the table rendering) accordingly.
290-299
: Dead / no‑op branches in cell‑border logicThe empty
else if (variable.isFirstTime) {}
and the empty terminalelse {}
add noise and make the reader wonder if something is missing.- } else if (variable.isFirstTime) { - } - } else { - } + }A quick prune improves clarity without altering behaviour.
315-324
: Accessibility: provide an aria‑label for the “secret” lock controlScreen‑reader users will only hear “button” (or nothing, depending on
SimpleTooltip
implementation). Addaria-label="Secret variable – value hidden"
(or wrap the existing text in a visually‑hidden span) so the purpose is conveyed without sight.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/webapp/app/components/primitives/CopyableText.tsx
(1 hunks)apps/webapp/app/components/primitives/Switch.tsx
(1 hunks)apps/webapp/app/components/primitives/Table.tsx
(1 hunks)apps/webapp/app/presenters/v3/EnvironmentVariablesPresenter.server.ts
(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables.new/route.tsx
(10 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx
(13 hunks)apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.$name.ts
(2 hunks)apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.ts
(1 hunks)apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
(7 hunks)apps/webapp/app/v3/environmentVariables/repository.ts
(2 hunks)internal-packages/database/prisma/migrations/20250411172850_environment_variable_value_is_secret/migration.sql
(1 hunks)internal-packages/database/prisma/schema.prisma
(1 hunks)packages/cli-v3/src/entryPoints/managed/heartbeat.ts
(1 hunks)packages/core/src/v3/apiClient/index.ts
(3 hunks)packages/core/src/v3/schemas/api.ts
(1 hunks)packages/trigger-sdk/src/v3/envvars.ts
(3 hunks)references/hello-world/src/trigger/envvars.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/core/src/v3/schemas/api.ts (1)
apps/webapp/app/v3/environmentVariables/repository.ts (1)
EnvironmentVariableWithSecret
(82-84)
packages/core/src/v3/apiClient/index.ts (2)
apps/webapp/app/v3/environmentVariables/repository.ts (1)
EnvironmentVariableWithSecret
(82-84)packages/core/src/v3/schemas/api.ts (2)
EnvironmentVariableWithSecret
(813-826)EnvironmentVariableWithSecret
(827-827)
apps/webapp/app/v3/environmentVariables/repository.ts (2)
packages/core/src/v3/schemas/api.ts (3)
EnvironmentVariableWithSecret
(813-826)EnvironmentVariableWithSecret
(827-827)EnvironmentVariable
(805-808)apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (1)
projectId
(574-594)
packages/trigger-sdk/src/v3/envvars.ts (4)
packages/core/src/v3/index.ts (1)
ApiPromise
(4-4)apps/webapp/app/v3/environmentVariables/repository.ts (1)
EnvironmentVariableWithSecret
(82-84)packages/core/src/v3/schemas/api.ts (2)
EnvironmentVariableWithSecret
(813-826)EnvironmentVariableWithSecret
(827-827)packages/core/src/v3/apiClient/index.ts (1)
ApiRequestOptions
(125-125)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
🔇 Additional comments (30)
apps/webapp/app/components/primitives/CopyableText.tsx (1)
54-54
: Tooltip text simplified for consistency.
The tooltip now displays “Copy” instead of “Copy text” when idle, aligning with the concise UI language used elsewhere. This update is clear and user-friendly.apps/webapp/app/components/primitives/Table.tsx (1)
352-354
: Introduce div wrapper with vertical dividers for hidden buttons.
WrappinghiddenButtons
in a flex container withdivide-x divide-grid-bright
cleanly separates the hover-only buttons with a vertical line. The implementation matches the updated design with no layout regressions.internal-packages/database/prisma/schema.prisma (1)
2732-2736
: DeclareisSecret
field inEnvironmentVariableValue
model.
The new booleanisSecret
is added with a default offalse
and a clear doc comment. The naming and location conform to existing model conventions.apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.ts (1)
83-86
: Good use of redacted secrets methodThe switch to
getEnvironmentWithRedactedSecrets
is appropriate here, ensuring that secret environment variable values are properly redacted before being returned to the client.apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.$name.ts (1)
126-129
: Good use of redacted secrets methodThe switch to
getEnvironmentWithRedactedSecrets
ensures that secret environment variable values are properly redacted when retrieved through this endpoint.packages/core/src/v3/apiClient/index.ts (3)
20-20
: Good schema importCorrectly importing the new
EnvironmentVariableWithSecret
schema to use in the API client methods.
553-553
: Updated return type for improved type safetyUpdating the return type of
listEnvVars
to use the newEnvironmentVariableWithSecret
schema ensures type safety and proper handling of secret environment variables in client applications.
583-583
:❓ Verification inconclusive
Updated return type for improved type safety
Updating the return type of
retrieveEnvVar
to use the newEnvironmentVariableWithSecret
schema ensures type safety and proper handling of secret environment variables in client applications.I notice that while the retrieval methods have been updated to use the new schema, the
createEnvVar
method (lines 593-609) still uses the originalCreateEnvironmentVariableRequestBody
schema that doesn't include theisSecret
flag. If creating secret environment variables is supported, the request body type might need to be updated.
🏁 Script executed:
#!/bin/bash # Check if there's an updated client method for creating environment variables with the isSecret flag grep -r "createEnvVar.*isSecret\|createSecretEnvVar" --include="*.ts" packages/Length of output: 81
🏁 Script executed:
#!/bin/bash # Locate the createEnvVar method and its usage of the request body type grep -R "createEnvVar" -n packages/core/src/v3/apiClient/index.ts # Locate the definition of CreateEnvironmentVariableRequestBody grep -R "type CreateEnvironmentVariableRequestBody" -n packages/Length of output: 252
🏁 Script executed:
#!/bin/bash # Extract createEnvVar method snippet for context sed -n '590,630p' packages/core/src/v3/apiClient/index.ts # Extract the CreateEnvironmentVariableRequestBody type definition sed -n '760,810p' packages/core/src/v3/schemas/api.tsLength of output: 2521
🏁 Script executed:
#!/bin/bash # Locate the EnvironmentVariableWithSecret schema definition grep -R "EnvironmentVariableWithSecret" -n packages/core/src/v3/schemas/api.tsLength of output: 233
Verify support for secret environment variable creation
The
CreateEnvironmentVariableRequestBody
schema (packages/core/src/v3/schemas/api.ts:772) currently only includes:export const CreateEnvironmentVariableRequestBody = z.object({ name: z.string(), value: z.string(), });…while retrieval now returns an
EnvironmentVariableWithSecret
(which includesisSecret
). If the API supports creating secret env vars, please verify and, if needed:
- Update the schema in packages/core/src/v3/schemas/api.ts:
export const CreateEnvironmentVariableRequestBody = z.object({ name: z.string(), value: z.string(), isSecret: z.boolean(), });- Adjust the client method signature in packages/core/src/v3/apiClient/index.ts (around line 593):
createEnvVar( projectRef: string, slug: string, body: CreateEnvironmentVariableRequestBody, // now with isSecret requestOptions?: ZodFetchOptions ) { … }If secret creation isn’t supported by the backend, please document that limitation or align the retrieval schema accordingly.
apps/webapp/app/v3/environmentVariables/repository.ts (4)
50-55
: Good addition of EditEnvironmentVariableValue schema.This new schema provides a clean way to handle editing a single environment variable value, which is a necessary addition for the new secret management functionality.
82-84
: Appropriate extension of EnvironmentVariable type.The
EnvironmentVariableWithSecret
type properly extends the base type with anisSecret
flag, maintaining backward compatibility while adding the new functionality.
89-97
: Repository interface correctly extended with new methods for secret management.The
editValue
andgetEnvironmentWithRedactedSecrets
methods are well-designed additions to the interface. The JSDoc comment clearly explains thatgetEnvironmentWithRedactedSecrets
returns environment variables with redacted secret values.
98-105
: Clear and helpful JSDoc comments for existing methods.The added JSDoc comments for
getEnvironment
andgetEnvironmentVariables
methods provide important clarification about their behavior, especially highlighting thatgetEnvironmentVariables
should only be used for executing tasks since it returns secret values.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables.new/route.tsx (7)
91-94
: Well-implemented schema preprocessing for isSecret flag.The preprocessing function correctly converts string inputs to boolean values, handling form data appropriately. This is a standard pattern for handling boolean values in forms.
202-202
: Changed default reveal behavior.The initial state of
revealAll
is now set totrue
. This makes sense given that users are creating new variables and need to see what they're typing.
217-219
: Improved dialog styling for better readability.The adjusted padding and max-width settings improve the layout and readability of the dialog.
261-273
: Well-designed UI for secret value toggle.The implementation of the secret toggle is clean and user-friendly. The hint text clearly explains that secret values cannot be viewed after creation, which is important for user understanding.
298-329
: Improved form button layout and styling.The restructured form buttons with better spacing and alignment improve the user experience. The explicit placement of buttons with a flex container provides better visual hierarchy.
336-337
: Simplified FieldLayout component.The refactored
FieldLayout
component is more concise and uses grid layout effectively for consistent alignment of form fields.
470-487
: Improved input field layout with flex container.The addition of a flex container for the value input and delete button creates a cleaner, more consistent UI layout that better accommodates the variable-width components.
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts (7)
8-8
: Good import for improved environment title handling.Using
environmentFullTitle
provides consistent naming across the application and improves error message clarity.
18-18
: Appropriate type import.Importing the
EnvironmentVariableWithSecret
type ensures type consistency between the interface and implementation.
55-55
: Added optional isSecret parameter to create method.This optional parameter allows backward compatibility while adding the new functionality.
132-133
: Improved error message with environment titles.Using
environmentFullTitle
instead of hardcoded environment titles provides more user-friendly error messages.
192-196
: Correctly implementing isSecret flag in create method.The implementation properly sets and updates the
isSecret
flag during environment variable creation, ensuring it's stored in the database.
361-438
: Well-implemented editValue method.The new
editValue
method is thoroughly implemented with proper validation and error handling. It follows the same pattern as other methods in the repository, ensuring consistency.One minor concern is that while the method updates the secret value, it doesn't provide a way to update the
isSecret
flag for an existing environment variable. Consider whether this is an intended limitation or if it should be addressed in a future update.Do you intend to allow users to change the
isSecret
status of an existing environment variable? If so, you might want to extend this method to update theisSecret
flag as well.
513-547
: Comprehensive implementation of getEnvironmentWithRedactedSecrets.The method correctly fetches environment variables, identifies which ones are marked as secret, and returns a properly redacted list. The redacted value is consistent ("") and the method preserves the original structure of non-secret variables.
packages/trigger-sdk/src/v3/envvars.ts (3)
7-7
: Good import of new type.Importing
EnvironmentVariableWithSecret
ensures type consistency throughout the SDK.
88-96
: Updated return types for list function.The
list
function return type is correctly updated to reflect that it now returns environment variables with secret information. The type changes are consistent across all function overloads.
194-204
: Updated return types for retrieve function.The
retrieve
function return type is correctly updated to reflect that it now returns an environment variable with secret information. The type changes are consistent across all function overloads.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx (1)
105-112
:❌ Incorrect review comment
Redundant / potentially conflicting shape properties in Zod union
DeleteEnvironmentVariableValue.shape
is already spread into thedelete
branch, yet an explicitkey
field is also declared beforehand.
IfDeleteEnvironmentVariableValue.shape
itself contains akey
definition (very likely), the manualkey
entry is silently overwritten – or worse, if the type differs, the earlier declaration wins and introduces an inconsistency that validation won't catch at compile‑time.- z.object({ - action: z.literal("delete"), - key: z.string(), - ...DeleteEnvironmentVariableValue.shape, - }), + z.object({ + action: z.literal("delete"), + ...DeleteEnvironmentVariableValue.shape, + }),Do a quick check on
EditEnvironmentVariableValue.shape
as well (the same duplication risk exists foraction
).
Cleaning this up removes ambiguity and future‑proofs the schema.
🏁 Script executed:
#!/bin/bash echo "Searching for DeleteEnvironmentVariableValue definition..." rg -C5 "DeleteEnvironmentVariableValue" -n . echo echo "Searching for EditEnvironmentVariableValue definition..." rg -C5 "EditEnvironmentVariableValue" -n .Length of output: 14539
No duplication:
DeleteEnvironmentVariableValue.shape
lacks akey
field
DeleteEnvironmentVariableValue
is defined as:export const DeleteEnvironmentVariableValue = z.object({ id: z.string(), environmentId: z.string(), });It does not include
key
, so the manualkey: z.string(),in the delete branch is both correct and required. Similarly,
EditEnvironmentVariableValue.shape
does not declareaction
, so there’s no overlap there either. You can safely ignore the earlier suggestion.Likely an incorrect or invalid review comment.
return json( | ||
variables.map((variable) => ({ | ||
name: variable.key, | ||
value: variable.value, | ||
isSecret: variable.isSecret, | ||
})) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Correctly exposing isSecret flag in API response
The response correctly includes the isSecret
flag alongside the variable name and value, providing clients with the information they need to display environment variables appropriately.
However, I notice that while the loader function has been updated to support retrieving secret environment variables, the action function (lines 15-60) doesn't appear to have been updated to allow setting the isSecret
flag on creation. This may be intentional if this ability is exposed through a different endpoint.
🏁 Script executed:
#!/bin/bash
# Check if environment variable creation with isSecret flag is handled elsewhere
grep -r "isSecret.*CreateEnvironmentVariableRequestBody" --include="*.ts" .
Length of output: 77
I haven’t found any references to isSecret
within the action handler yet—let’s pull in the full action function and check for any creation logic that handles secrets:
This will show whether the action block includes any handling of the isSecret
flag. If nothing turns up, you’ll need to manually verify or update the action to accept and persist isSecret
on variable creation.
🏁 Script executed:
#!/bin/bash
# Locate the action export and search within it for isSecret usage
rg -n "export const action" -g 'apps/webapp/app/routes/api.v1.projects.*.envvars.*.ts' -C5
rg -n "isSecret" -g 'apps/webapp/app/routes/api.v1.projects.*.envvars.*.ts'
Length of output: 396
Let’s inspect the action in the slug route to see if it ever reads or forwards an isSecret
value:
🏁 Script executed:
#!/bin/bash
# Identify the slug-only file (exclude the $name file)
files=(apps/webapp/app/routes/api.v1.projects.*.envvars.*.ts)
for f in "${files[@]}"; do
if [[ "$f" != *"\$name.ts" ]]; then
slug_file="$f"
fi
done
echo "Inspecting: $slug_file"
echo
# Show all exports (loader + action)
rg -n "^export" -g "$slug_file" -C5
echo
# Dump the top 100 lines to capture the action definition
sed -n '1,100p' "$slug_file"
Length of output: 5969
Action handler must forward isSecret
when creating variables
The current action
in apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.ts
only reads name
and value
from the request and calls:
repository.create(projectId, {
override: true,
environmentIds: [environment.id],
variables: [
{
key: body.data.name,
value: body.data.value,
},
],
});
As a result, new variables can’t be marked secret on creation. To fix:
- Extend the
CreateEnvironmentVariableRequestBody
schema to includeisSecret: boolean
. - In the
action
function, includebody.data.isSecret
in the payload:variables: [ {
-
key: body.data.name,
-
value: body.data.value,
-
key: body.data.name,
-
value: body.data.value,
-
isSecret: body.data.isSecret, }, ],
- Confirm that
EnvironmentVariablesRepository.create()
accepts and persistsisSecret
(update its signature if necessary).
This change aligns creation with the loader’s redacted‐secrets behavior and ensures clients can set the secret flag.
return json({ | ||
name: environmentVariable.key, | ||
value: environmentVariable.value, | ||
isSecret: environmentVariable.isSecret, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Properly exposing isSecret flag in API response
The response now correctly includes the isSecret
flag, which allows clients to handle secret environment variables appropriately in their UI.
Similar to the previous file, I notice that the action function for updating environment variables (lines 73-82) doesn't appear to have been modified to support updating the isSecret
flag. This may be intentional if the secret status can only be set during creation or if it's handled through a different endpoint.
🏁 Script executed:
#!/bin/bash
# Check if environment variable updating with isSecret flag is handled elsewhere
grep -r "isSecret.*edit.*environmentVariable" --include="*.ts" .
Length of output: 66
I’ll locate and inspect the update action in the envvars route to see whether isSecret
is being handled there:
🏁 Script executed:
#!/bin/bash
# Find the envvars route file and dump its first 200 lines
FILE=$(fd 'api.v1.projects' -e ts | grep envvars | head -n1)
echo "Inspecting $FILE"
sed -n '1,200p' "$FILE"
Length of output: 4494
I’ll inspect the repository implementation to see if edit
accepts an isSecret
option:
#!/bin/bash
# Locate the repository file and show its edit method signature
REPO_FILE=$(fd environmentVariablesRepository.server -e ts | head -n1)
echo "Inspecting $REPO_FILE"
grep -n "edit(" "$REPO_FILE"
sed -n '1,200p' "$REPO_FILE"
I’ll check the EnvironmentVariablesRepository
to see if its edit
method supports an isSecret
option:
#!/bin/bash
# Locate the repository file and inspect its edit method signature
REPO_FILE=$(fd "environmentVariablesRepository.server.ts" -e ts | head -n1)
echo "Inspecting $REPO_FILE"
grep -R "edit" -n "$REPO_FILE"
sed -n '1,200p' "$REPO_FILE"
I’ll locate the repository implementation and inspect its edit
method signature and parameters:
🏁 Script executed:
#!/bin/bash
# Find the EnvironmentVariablesRepository file
REPO_FILE=$(rg -l "class EnvironmentVariablesRepository" -g "*.ts" | head -n1)
echo "Inspecting $REPO_FILE"
# Show method signature for edit
grep -R "edit(" -n "$REPO_FILE" | sed -n '1,10p'
Length of output: 490
I'll extract the edit
method signature and look for any isSecret
handling:
#!/bin/bash
FILE="apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts"
echo "=== edit method signature (lines 220–260) ==="
sed -n '220,260p' "$FILE"
echo
echo "=== Any references to isSecret in this file ==="
grep -R "isSecret" -n "$FILE" || echo "No matches"
Include isSecret
in the update action
The PUT/POST handler currently only parses and forwards value
to repository.edit
, so clients cannot toggle the secret flag after creation. To support updating isSecret
, you’ll need to:
- Extend the request schema (
UpdateEnvironmentVariableRequestBody
) to includeisSecret
. - Pass
isSecret: body.data.isSecret
into theedit
call in
apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.$name.ts
(around lines 73–82). - Update the
EnvironmentVariablesRepository.edit
method in
apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts
to accept and persist theisSecret
flag.
If the secret status truly must remain immutable after creation, please document that constraint in the API spec.
onInterval: async () => { | ||
this.logger.sendDebugLog({ | ||
runId: this.runFriendlyId, | ||
message: "heartbeat: started", | ||
}); | ||
|
||
const response = await this.httpClient.heartbeatRun( | ||
this.runFriendlyId, | ||
this.snapshotFriendlyId | ||
); | ||
|
||
if (!response.success) { | ||
this.logger.sendDebugLog({ | ||
runId: this.runFriendlyId, | ||
message: "heartbeat: failed", | ||
properties: { | ||
error: response.error, | ||
}, | ||
}); | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Wrap the heartbeat call in its own try/catch to guarantee error capture
If httpClient.heartbeatRun
throws (e.g. network failure), the promise rejects before you can write the “failed” debug log, and the error bubbles up to IntervalService
’s onError
.
Capturing the exception locally lets you log both the rejection and the non‑success response uniformly:
-const response = await this.httpClient.heartbeatRun(
- this.runFriendlyId,
- this.snapshotFriendlyId
-);
-
-if (!response.success) {
+try {
+ const response = await this.httpClient.heartbeatRun(
+ this.runFriendlyId,
+ this.snapshotFriendlyId
+ );
+
+ if (!response.success) {
this.logger.sendDebugLog({
runId: this.runFriendlyId,
message: "heartbeat: failed",
properties: {
error: response.error,
},
});
}
+} catch (err) {
+ this.logger.sendDebugLog({
+ runId: this.runFriendlyId,
+ message: "heartbeat: exception",
+ properties: { error: err instanceof Error ? err.message : String(err) },
+ });
+ throw err; // still delegate to IntervalService.onError
+}
📝 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.
onInterval: async () => { | |
this.logger.sendDebugLog({ | |
runId: this.runFriendlyId, | |
message: "heartbeat: started", | |
}); | |
const response = await this.httpClient.heartbeatRun( | |
this.runFriendlyId, | |
this.snapshotFriendlyId | |
); | |
if (!response.success) { | |
this.logger.sendDebugLog({ | |
runId: this.runFriendlyId, | |
message: "heartbeat: failed", | |
properties: { | |
error: response.error, | |
}, | |
}); | |
} | |
}, | |
onInterval: async () => { | |
this.logger.sendDebugLog({ | |
runId: this.runFriendlyId, | |
message: "heartbeat: started", | |
}); | |
try { | |
const response = await this.httpClient.heartbeatRun( | |
this.runFriendlyId, | |
this.snapshotFriendlyId | |
); | |
if (!response.success) { | |
this.logger.sendDebugLog({ | |
runId: this.runFriendlyId, | |
message: "heartbeat: failed", | |
properties: { | |
error: response.error, | |
}, | |
}); | |
} | |
} catch (err) { | |
this.logger.sendDebugLog({ | |
runId: this.runFriendlyId, | |
message: "heartbeat: exception", | |
properties: { error: err instanceof Error ? err.message : String(err) }, | |
}); | |
throw err; // still delegate to IntervalService.onError | |
} | |
}, |
{...conform.input(value, { type: "text" })} | ||
placeholder={variable.isSecret ? "Set new secret value" : "Not set"} | ||
defaultValue={variable.value} | ||
type={"text"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Secret values should use type="password"
when editing
Even though the placeholder hints that the current value cannot be revealed, the input is rendered as plain text. Using type="password"
prevents accidental shoulder‑surfing and disables browser auto‑fill heuristics.
- {...conform.input(value, { type: "text" })}
+ {...conform.input(value, { type: "password" })}
You can keep it as "text"
for non‑secret variables by switching on variable.isSecret
.
Committable suggestion skipped: line range outside the PR's diff.
You can now mark an env var as "secret" when you create it. Then you won't be able to see the value again in the dashboard.
We've also reworked the design of the list page, creation and editing panels. This prepares us for when we add Preview branch support, which requires us to have env vars set multiple times with overrides.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
API & SDK Updates
isSecret
flag for environment variables.Chores
isSecret
field for environment variable values.