feat: env var in flow #6852

dieriba · 2025-10-17T09:22:18Z

No description provided.

claude · 2025-10-17T09:22:48Z

Claude finished @dieriba's task —— View job

Code Review Complete ✅

I've completed a comprehensive review of PR #6852: feat: env var in flow

Overview

This PR adds support for flow-level environment variables, allowing users to define key-value pairs at the flow level that can be referenced in flow steps via env.VARIABLE_NAME. This is a valuable feature for managing flow-specific configuration.

Summary of Changes

Backend (Rust):

Added env_vars: Option<HashMap<String, String>> field to FlowValue struct
Modified transform_input function to accept and inject environment variables into the evaluation context
Updated all call sites of transform_input to pass flow.env_vars.as_ref()
Updated OpenAPI specification to document the new field

Frontend (Svelte):

Created new FlowEnvironmentVariables.svelte component (replacing deleted FlowConstants.svelte)
Integrated environment variables into the property picker system
Added UI for adding, editing, and removing environment variables
Made env vars available in PropPicker and PropPickerResult components

Code Quality Assessment

✅ Strengths

Clean architecture: The feature integrates well with existing code patterns
Proper data flow: Environment variables are correctly threaded through backend execution
Good UX: Environment variables are discoverable in the property picker
Documentation: OpenAPI spec is updated
Type safety: Uses appropriate Rust types (Option<HashMap<String, String>>)

⚠️ Issues & Recommendations

High Priority

Input Validation Missing (FlowEnvironmentVariables.svelte:57)
- Variable names aren't validated (should be valid identifiers)
- Duplicate key detection is missing
- Could lead to user confusion or errors
Error Handling (worker_flow.rs:1873)
- Serialization of env_args could panic
- Should use proper error handling with Result type

Medium Priority

Performance Concern (FlowEnvironmentVariables.svelte:23)
- $derived recreates array on every access
- Should use $derived.by() for memoization
UX Clarity (PropPicker.svelte:426)
- "Flow Variables" label could be confused with platform Variables
- Consider "Flow Environment Variables" or "Flow Env Vars"
State Management Issue (FlowEnvironmentVariables.svelte:102)
- Input loses focus/value on invalid key entry
- Should use controlled input pattern

Low Priority

Documentation (worker_flow.rs:1849)
- Add comment explaining these are flow-level (not system) env vars

Security Considerations

✅ No security issues identified

Environment variables are properly scoped to flows
No credential exposure risks
Values are stored in flow definitions (not secrets)

Performance Considerations

✅ Generally good performance

Minor optimization opportunity in frontend (see inline comment Give clear self-host instructions #3)
Backend changes are efficient (passing references, not cloning)

Testing Recommendations

Test environment variable resolution in different flow step types (script, subflow, branches, loops)
Test special characters and edge cases in variable names
Test empty values and very long values
Test that env vars are properly isolated between flows
Verify property picker autocomplete works correctly
Test that deleting the flow cleans up env vars properly

Missing Considerations

Database migration: Does the schema change require a migration? (Appears to be JSON field, so probably not)
Backward compatibility: Old flows without env_vars will work fine (it's Option)
Documentation: User-facing docs should be updated to explain this feature
Tests: No test files were included in this PR

Reproduction Instructions for Testing

To test the new flow environment variables feature, navigate to the Flow Editor in the Windmill UI. Open an existing flow or create a new one, then click on the flow settings or configuration panel where you should see a new "Environment Variables" section (likely in a sidebar or settings tab alongside other flow-level settings like "Input", "Settings", or "Constants"). In this section, click the "Add Variable" button to create a new environment variable entry. Enter a variable name (e.g., API_BASE_URL) in the first input field and a corresponding value (e.g., https://api.example.com) in the second field. Save the flow. Now, in any flow step (script, inline code, or transform), you can reference this variable using the syntax env.API_BASE_URL - for example, in a JavaScript expression you might write return env.API_BASE_URL + '/users'. The property picker should show your environment variables under a "Flow Variables" or similar section when you're editing step inputs, making them easily discoverable. Test that the values are correctly resolved during flow execution by running the flow and checking that your scripts receive the expected environment variable values. Also verify that you can edit variable names and values, remove variables, and that these changes persist when you save and reload the flow.

Recommendation

Approve with minor changes requested

The core implementation is solid and the feature is well-integrated. However, I recommend addressing the input validation issue (#1) and error handling issue (#2) before merging, as these could lead to user-facing bugs. The other issues are nice-to-haves that can be addressed in follow-up PRs if needed.

Inline comments: 9 comments posted on specific lines

claude · 2025-10-17T09:24:35Z

backend/windmill-worker/src/worker_flow.rs

 /// resumes should be in order of timestamp ascending, so that more recent are at the end
 #[instrument(level = "trace", skip_all)]
 async fn transform_input(
+    env_args: Option<&HashMap<String, String>>,


Good addition: The env_args parameter properly passes flow environment variables to the transform function. However, consider adding a comment explaining that these are flow-level environment variables (not system env vars) for clarity.

Suggested change

env_args: Option<&HashMap<String, String>>,

async fn transform_input(

// Flow-level environment variables defined in the flow configuration

env_args: Option<&HashMap<String, String>>,

claude · 2025-10-17T09:24:37Z

backend/windmill-worker/src/worker_flow.rs

        env.insert("resume".to_string(), resume);
        env.insert("resumes".to_string(), resumes);
        env.insert("approvers".to_string(), approvers);
+        if let Some(env_args) = env_args {


Potential issue: Consider using serde_json::to_value() instead of to_raw_value() for better type safety and error handling. The current implementation might panic on serialization failure.

Suggested change

if let Some(env_args) = env_args {

if let Some(env_args) = env_args {

match serde_json::to_value(env_args) {

Ok(val) => {

if let Ok(raw) = serde_json::value::to_raw_value(&val) {

env.insert("env".to_string(), Arc::new(raw));

}

}

Err(e) => {

tracing::warn!("Failed to serialize env_args: {}", e);

}

}

}

claude · 2025-10-17T09:24:38Z

frontend/src/lib/components/flows/content/FlowEnvironmentVariables.svelte

+		flowStore.val.value.env_vars = {}
+	}
+	let envVars = $derived(flowStore.val.value.env_vars || {})
+	let envEntries = $derived(


Svelte 5 best practice: Good use of $derived for reactive state! However, consider memoizing the envEntries computation since Object.entries() creates a new array on every access. This could cause unnecessary re-renders.

Suggested change

let envEntries = $derived(

let envEntries = $derived.by(() =>

Object.entries(envVars).map(([key, value], index) => ({ id: `${key}_${index}`, key, value }))

)

claude · 2025-10-17T09:24:40Z

frontend/src/lib/components/flows/content/FlowEnvironmentVariables.svelte

+		}
+	}
+
+	function updateEnvKey(oldKey: string, newKey: string) {


Potential bug: This function mutates the store but doesn't validate the new key. If newKey already exists or contains invalid characters, it could cause issues. Add validation:

Suggested change

function updateEnvKey(oldKey: string, newKey: string) {

function updateEnvKey(oldKey: string, newKey: string) {

if (flowStore.val.value.env_vars && oldKey !== newKey && newKey.trim() !== '') {

// Validate key format (alphanumeric, underscore, starts with letter)

if (!/^[A-Z_][A-Z0-9_]*$/i.test(newKey)) {

// Show error to user

return

}

// Check for duplicate

if (newKey in flowStore.val.value.env_vars) {

// Show error to user

return

}

const value = flowStore.val.value.env_vars[oldKey]

delete flowStore.val.value.env_vars[oldKey]

flowStore.val.value.env_vars[newKey] = value

flowStore.val = flowStore.val

}

}

claude · 2025-10-17T09:24:41Z

frontend/src/lib/components/flows/content/FlowEnvironmentVariables.svelte

+								<input
+									type="text"
+									value={entry.key}
+									onblur={(e) => {


Svelte 5 compliance issue: You're using onblur with a function, but in Svelte 5 you should use the direct event attribute syntax. However, the more critical issue is that this loses the old key value if the user enters an invalid or duplicate key. Consider using a controlled input pattern with temporary state.

claude · 2025-10-17T09:24:42Z

frontend/src/lib/components/propertyPicker/PropPicker.svelte

 	export let allowCopy = false
 	export let alwaysOn = false
 	export let previousId: string | undefined = undefined
+	export let env: Record<string, string> | undefined = undefined


Good addition: The env prop is properly passed through and made available in the property picker. This provides good UX for discovering available environment variables.

claude · 2025-10-17T09:24:43Z

frontend/src/lib/components/propertyPicker/PropPicker.svelte

 			{/if}
+			{#if env && Object.keys(env).length > 0 && (!filterActive || $inputMatches?.some((match) => match.word === 'env'))}
+				<div class="overflow-y-auto pb-2">
+					<span class="font-normal text-xs text-secondary">Flow Variables:</span>


Nitpick: The label "Flow Variables" might be confusing since the platform also has "Variables" as a separate concept. Consider using "Flow Environment Variables" or "Flow Env Vars" for clarity.

claude · 2025-10-17T09:24:44Z

backend/windmill-common/src/flows.rs

    pub chat_input_enabled: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(default)]
+    pub env_vars: Option<HashMap<String, String>>,


Good implementation: The env_vars field is properly added to the FlowValue struct with appropriate serialization attributes. The Option<HashMap<String, String>> type is correct for optional key-value pairs.

claude · 2025-10-17T09:24:45Z

openflow.openapi.yaml

          type: string
        cache_ttl:
          type: number
+        env_vars:


Good documentation: The OpenAPI spec is properly updated to reflect the new env_vars field. This ensures API documentation stays in sync with the code.

cloudflare-workers-and-pages · 2025-10-17T14:51:53Z

Deploying windmill with Cloudflare Pages

Latest commit:	`baaf3c4`
Status:	✅ Deploy successful!
Preview URL:	https://e390c93f.windmill.pages.dev
Branch Preview URL:	https://dieri-env-var-in-flow.windmill.pages.dev

View logs

dieriba added 3 commits October 16, 2025 13:36

ok

a5fd0c2

Merge remote-tracking branch 'refs/remotes/origin/main'

755e3b5

done

d7f27a8

dieriba requested review from alpetric, hugocasa and rubenfiszel as code owners October 17, 2025 09:22

claude bot reviewed Oct 17, 2025

View reviewed changes

dieriba added 2 commits October 17, 2025 16:31

subflow

f5b81f6

nits

d57f99c

dieriba added 7 commits October 17, 2025 23:59

ok

12c7484

Merge branch 'main' into dieri/env-var-in-flow

2665914

good env

63cfa9f

Merge branch 'main' into dieri/env-var-in-flow

1dddac3

update sqlx

4555d12

nits

955a7f6

lazy

baaf3c4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: env var in flow #6852

feat: env var in flow #6852

Uh oh!

dieriba commented Oct 17, 2025

Uh oh!

claude bot commented Oct 17, 2025 •

edited

Loading

Uh oh!

claude bot Oct 17, 2025

Uh oh!

claude bot Oct 17, 2025

Uh oh!

claude bot Oct 17, 2025

Uh oh!

claude bot Oct 17, 2025

Uh oh!

claude bot Oct 17, 2025

Uh oh!

claude bot Oct 17, 2025

Uh oh!

claude bot Oct 17, 2025

Uh oh!

claude bot Oct 17, 2025

Uh oh!

claude bot Oct 17, 2025

Uh oh!

cloudflare-workers-and-pages bot commented Oct 17, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-        if let Some(env_args) = env_args {
+        if let Some(env_args) = env_args {
+            match serde_json::to_value(env_args) {
+                Ok(val) => {
+                    if let Ok(raw) = serde_json::value::to_raw_value(&val) {
+                        env.insert("env".to_string(), Arc::new(raw));
+                    }
+                }
+                Err(e) => {
+                    tracing::warn!("Failed to serialize env_args: {}", e);
+                }
+            }
+        }

-	let envEntries = $derived(
+	let envEntries = $derived.by(() =>
+		Object.entries(envVars).map(([key, value], index) => ({ id: `${key}_${index}`, key, value }))
+	)

-	function updateEnvKey(oldKey: string, newKey: string) {
+	function updateEnvKey(oldKey: string, newKey: string) {
+		if (flowStore.val.value.env_vars && oldKey !== newKey && newKey.trim() !== '') {
+			// Validate key format (alphanumeric, underscore, starts with letter)
+			if (!/^[A-Z_][A-Z0-9_]*$/i.test(newKey)) {
+				// Show error to user
+				return
+			}
+			// Check for duplicate
+			if (newKey in flowStore.val.value.env_vars) {
+				// Show error to user
+				return
+			}
+			const value = flowStore.val.value.env_vars[oldKey]
+			delete flowStore.val.value.env_vars[oldKey]
+			flowStore.val.value.env_vars[newKey] = value
+			flowStore.val = flowStore.val
+		}
+	}

feat: env var in flow #6852

Are you sure you want to change the base?

feat: env var in flow #6852

Uh oh!

Conversation

dieriba commented Oct 17, 2025

Uh oh!

claude bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✅

Overview

Summary of Changes

Code Quality Assessment

✅ Strengths

⚠️ Issues & Recommendations

High Priority

Medium Priority

Low Priority

Security Considerations

Performance Considerations

Testing Recommendations

Missing Considerations

Reproduction Instructions for Testing

Recommendation

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

cloudflare-workers-and-pages bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying windmill with Cloudflare Pages

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

claude bot commented Oct 17, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 17, 2025 •

edited

Loading