Dynamic Global Variables and Secrets Management - #10
Conversation
This commit introduces comprehensive client-side dynamic Globals and Secrets management for the agent flow canvas.
Key changes:
- Created `GlobalsManager.tsx` UI editor modal replicating the workspace gateway style.
- Created `globals.ts` to manage client-side state under `agent_flow.globals` and `agent_flow.secrets` in localStorage.
- Integrated `{{global.KEY}}` and `{{secret.KEY}}` interpolation inside `runFlow.ts` browser execution.
- Added variable key validation (`/^[a-zA-Z_][a-zA-Z0-9_]*$/`) and secret visibility toggling.
- Enhanced Python and JavaScript code generation (`codegen.ts`) to declare globals/secrets and resolve them dynamically.
- Implemented robust unit testing in `globalsAndSecrets.test.ts`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds persisted global and secret variables with an editor UI, runtime template interpolation across workflow nodes, generated Python and JavaScript support, canvas integration, and end-to-end tests. ChangesGlobals and secrets
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GlobalsManager
participant Index
participant runFlow
participant HTTP
User->>GlobalsManager: Edit global or secret
GlobalsManager->>Index: Update variable state
Index->>runFlow: Execute flow with variables
runFlow->>HTTP: Send interpolated URL, headers, and body
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
frontend/src/flow/globals.ts (1)
27-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate load/save logic for globals vs. secrets.
loadGlobals/saveGlobalsandloadSecrets/saveSecretsare structurally identical aside from the storage key and type. Extracting a generic helper reduces duplication and gives one place to add shape validation for parsed items (currentlyArray.isArray(parsed)is the only check — malformed/hand-edited localStorage entries with non-stringkey/valuewould silently propagate and could throw downstream, e.g. inGlobalsManager.tsx'skey.trim()).♻️ Suggested refactor
-export function loadGlobals(): GlobalVar[] { - try { - const raw = localStorage.getItem(GLOBALS_KEY); - if (raw) { - const parsed = JSON.parse(raw); - if (Array.isArray(parsed)) return parsed; - } - } catch { - /* ignore */ - } - return []; -} - -export function saveGlobals(items: GlobalVar[]) { - try { - localStorage.setItem(GLOBALS_KEY, JSON.stringify(items)); - } catch { - /* ignore */ - } -} - -export function loadSecrets(): SecretVar[] { - try { - const raw = localStorage.getItem(SECRETS_KEY); - if (raw) { - const parsed = JSON.parse(raw); - if (Array.isArray(parsed)) return parsed; - } - } catch { - /* ignore */ - } - return []; -} - -export function saveSecrets(items: SecretVar[]) { - try { - localStorage.setItem(SECRETS_KEY, JSON.stringify(items)); - } catch { - /* ignore */ - } -} +function loadItems<T>(key: string): T[] { + try { + const raw = localStorage.getItem(key); + if (raw) { + const parsed = JSON.parse(raw); + if (Array.isArray(parsed)) return parsed; + } + } catch { + /* ignore */ + } + return []; +} + +function saveItems<T>(key: string, items: T[]) { + try { + localStorage.setItem(key, JSON.stringify(items)); + } catch { + /* ignore */ + } +} + +export const loadGlobals = () => loadItems<GlobalVar>(GLOBALS_KEY); +export const saveGlobals = (items: GlobalVar[]) => saveItems(GLOBALS_KEY, items); +export const loadSecrets = () => loadItems<SecretVar>(SECRETS_KEY); +export const saveSecrets = (items: SecretVar[]) => saveItems(SECRETS_KEY, items);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/flow/globals.ts` around lines 27 - 67, Extract the duplicated localStorage read/write behavior from loadGlobals, saveGlobals, loadSecrets, and saveSecrets into generic helpers parameterized by storage key and item type. In the shared load helper, retain the empty-array fallback and validate each parsed entry’s key and value are strings before returning it, filtering malformed entries; have the globals and secrets functions delegate to these helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/flow/codegen.ts`:
- Around line 208-211: Update both interpolation sites in
frontend/src/flow/codegen.ts: lines 208-211 in the Python generator and lines
497-502 in the JavaScript generator. Replace exact placeholder substitutions
with escaped-key regular expressions that tolerate whitespace around global and
secret names, while replacing all matches and preserving values safely during
substitution.
In `@frontend/src/flow/globals.ts`:
- Around line 1-3: Update the GlobalsManager.tsx info panel to explicitly warn
that secrets are stored unencrypted in localStorage, are accessible to scripts
on the same origin, and persist on the device; advise users not to enter
production credentials. Keep the existing syntax guidance and do not alter
saveSecrets/loadSecrets unless encryption is specifically implemented.
In `@frontend/src/flow/GlobalsManager.tsx`:
- Around line 39-53: Update validateKey to accept the id of the item being
validated and exclude matches using that parameter instead of the closed-over
selectedId. Update both call sites to pass the corresponding id: selected.id for
the selected item and item.id for each sidebar row, preserving duplicate
detection across all globals and secrets.
- Around line 39-44: Update validateKey in GlobalsManager so the identifier
regex validates the raw key value rather than key.trim(), causing leading or
trailing whitespace to be rejected. Preserve the existing required-key check and
error messages, and ensure the updateSelected flow cannot persist
whitespace-padded keys as valid identifiers.
In `@frontend/src/flow/runFlow.ts`:
- Around line 56-66: Escape g.key and s.key before interpolating them into the
RegExp constructors in the global and secret replacement loops, or replace both
dynamic patterns with static regexes and callback-based lookups. Ensure keys
containing regex metacharacters are matched literally while preserving the
existing placeholder replacement behavior.
---
Nitpick comments:
In `@frontend/src/flow/globals.ts`:
- Around line 27-67: Extract the duplicated localStorage read/write behavior
from loadGlobals, saveGlobals, loadSecrets, and saveSecrets into generic helpers
parameterized by storage key and item type. In the shared load helper, retain
the empty-array fallback and validate each parsed entry’s key and value are
strings before returning it, filtering malformed entries; have the globals and
secrets functions delegate to these helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f9ac7ba-7f4c-4ef4-aec5-efda17bec2f7
⛔ Files ignored due to path filters (1)
server.logis excluded by!**/*.log
📒 Files selected for processing (7)
frontend/src/flow/GlobalsManager.tsxfrontend/src/flow/codegen.tsfrontend/src/flow/globals.tsfrontend/src/flow/runFlow.tsfrontend/src/pages/Index.tsxfrontend/src/test/codegen.test.tsfrontend/src/test/globalsAndSecrets.test.ts
| lines.push(` for k, v in GLOBALS.items():`); | ||
| lines.push(` res = res.replace(f"{{{{global.{k}}}}}", str(v))`); | ||
| lines.push(` for k, v in SECRETS.items():`); | ||
| lines.push(` res = res.replace(f"{{{{secret.{k}}}}}", str(v))`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align generated code interpolation with whitespace-tolerant runtime behavior.
The workflow runtime engine (runFlow.ts) uses a regex with \s* to allow flexible whitespace within placeholders (e.g., {{ global.KEY }}). However, both the Python and JavaScript code generators strictly replace exact {{global.KEY}} strings and fall back to stripping out whitespace-padded placeholders without interpolating them.
This creates a discrepancy where a workflow that functions perfectly in the visual builder will evaluate placeholders to empty strings in the exported scripts. Update both generators to use regex-based interpolation that aligns with the visual execution logic.
frontend/src/flow/codegen.ts#L208-L211: In the Python generator, replace exact string replacements with a safere.subimplementation:for k, v in GLOBALS.items(): pattern = r"\{\{\s*global\." + re.escape(k) + r"\s*\}\}" res = re.sub(pattern, lambda m, val=v: str(val), res) for k, v in SECRETS.items(): pattern = r"\{\{\s*secret\." + re.escape(k) + r"\s*\}\}" res = re.sub(pattern, lambda m, val=v: str(val), res)
frontend/src/flow/codegen.ts#L497-L502: In the JavaScript generator, replacereplaceAllwith regex replacements:for (const [k, v] of Object.entries(GLOBALS)) { const escapedKey = k.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); res = res.replace(new RegExp(`\\{\\{\\s*global\\.${escapedKey}\\s*\\}\\}`, "g"), String(v)); } for (const [k, v] of Object.entries(SECRETS)) { const escapedKey = k.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); res = res.replace(new RegExp(`\\{\\{\\s*secret\\.${escapedKey}\\s*\\}\\}`, "g"), String(v)); }
📍 Affects 1 file
frontend/src/flow/codegen.ts#L208-L211(this comment)frontend/src/flow/codegen.ts#L497-L502
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/flow/codegen.ts` around lines 208 - 211, Update both
interpolation sites in frontend/src/flow/codegen.ts: lines 208-211 in the Python
generator and lines 497-502 in the JavaScript generator. Replace exact
placeholder substitutions with escaped-key regular expressions that tolerate
whitespace around global and secret names, while replacing all matches and
preserving values safely during substitution.
| // Globals and Secrets Management | ||
| // Stored in localStorage under "agent_flow.globals" and "agent_flow.secrets" | ||
| // Values are stored client-side only. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Secrets are persisted in plaintext in localStorage.
SecretVar.value is written and read as plain text via saveSecrets/loadSecrets, with no encryption. Anything in localStorage is readable by any script running on the same origin (XSS, malicious/compromised dependency, browser extensions) and persists indefinitely on the device. This is a well-known anti-pattern for API keys/credentials.
At minimum, surface this tradeoff explicitly to users in the GlobalsManager.tsx info panel (it currently only documents syntax, not storage risk) so users don't unknowingly paste production credentials. If this tool is meant to also handle production-grade secrets, consider encrypting at rest with the Web Crypto API.
Also applies to: 48-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/flow/globals.ts` around lines 1 - 3, Update the
GlobalsManager.tsx info panel to explicitly warn that secrets are stored
unencrypted in localStorage, are accessible to scripts on the same origin, and
persist on the device; advise users not to enter production credentials. Keep
the existing syntax guidance and do not alter saveSecrets/loadSecrets unless
encryption is specifically implemented.
| const validateKey = (key: string): string | null => { | ||
| const trimmed = key.trim(); | ||
| if (!trimmed) return "Key is required"; | ||
| if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(trimmed)) { | ||
| return "Key must be a valid identifier (alphanumeric & underscore, start with letter/underscore)"; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Whitespace in a key passes validation but breaks placeholder interpolation.
validateKey tests key.trim() against the identifier regex, but updateSelected({ key: e.target.value }) (line 264) persists the raw, untrimmed value. A key like "KEY " passes validation (since the trimmed form is a valid identifier) and is saved as-is, but {{global.KEY}}/{{secret.KEY}} placeholders in node config won't match the stored key (which has a trailing space), so the generated Python/JS interpolate() functions silently fail to substitute it.
Validate the raw string instead of the trimmed one so whitespace is rejected, rather than silently accepted:
- if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(trimmed)) {
+ if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(key)) {Also applies to: 262-272
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/flow/GlobalsManager.tsx` around lines 39 - 44, Update
validateKey in GlobalsManager so the identifier regex validates the raw key
value rather than key.trim(), causing leading or trailing whitespace to be
rejected. Preserve the existing required-key check and error messages, and
ensure the updateSelected flow cannot persist whitespace-padded keys as valid
identifiers.
| const validateKey = (key: string): string | null => { | ||
| const trimmed = key.trim(); | ||
| if (!trimmed) return "Key is required"; | ||
| if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(trimmed)) { | ||
| return "Key must be a valid identifier (alphanumeric & underscore, start with letter/underscore)"; | ||
| } | ||
| // Check for duplicate keys of the same type or cross-type duplicate | ||
| const matches = items.filter( | ||
| (item) => item.key.trim().toLowerCase() === trimmed.toLowerCase() && item.id !== selectedId | ||
| ); | ||
| if (matches.length > 0) { | ||
| return "Key must be unique across all globals & secrets"; | ||
| } | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
validateKey uses the wrong id when called for non-selected items — every other row will show a false "duplicate key" warning.
validateKey excludes matches by the closed-over selectedId, not by the id of the item actually being validated. This works correctly at line 57 (validateKey(selected.key), where selectedId === selected.id), but at line 175 it's called per sidebar row as validateKey(item.key). For any row where item.id !== selectedId, the filter only excludes the currently-selected item — it never excludes item itself — so item always self-matches its own key and matches.length > 0 is always true. Every unselected row will permanently show the "⚠ invalid key" warning regardless of whether an actual duplicate exists.
🐛 Proposed fix
- const validateKey = (key: string): string | null => {
+ const validateKey = (key: string, id: string | null): string | null => {
const trimmed = key.trim();
if (!trimmed) return "Key is required";
if (!/^[a-zA-Z_][a-zA-Z0-9_]*$/.test(trimmed)) {
return "Key must be a valid identifier (alphanumeric & underscore, start with letter/underscore)";
}
// Check for duplicate keys of the same type or cross-type duplicate
const matches = items.filter(
- (item) => item.key.trim().toLowerCase() === trimmed.toLowerCase() && item.id !== selectedId
+ (item) => item.key.trim().toLowerCase() === trimmed.toLowerCase() && item.id !== id
);
if (matches.length > 0) {
return "Key must be unique across all globals & secrets";
}
return null;
};And update both call sites: validateKey(selected.key, selected.id) (line 57) and validateKey(item.key, item.id) (line 175).
Also applies to: 173-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/flow/GlobalsManager.tsx` around lines 39 - 53, Update
validateKey to accept the id of the item being validated and exclude matches
using that parameter instead of the closed-over selectedId. Update both call
sites to pass the corresponding id: selected.id for the selected item and
item.id for each sidebar row, preserving duplicate detection across all globals
and secrets.
| globals.forEach((g) => { | ||
| const regex = new RegExp(`\\{\\{\\s*global\\.${g.key}\\s*\\}\\}`, "g"); | ||
| result = result.replace(regex, g.value); | ||
| }); | ||
| } | ||
|
|
||
| if (secrets) { | ||
| secrets.forEach((s) => { | ||
| const regex = new RegExp(`\\{\\{\\s*secret\\.${s.key}\\s*\\}\\}`, "g"); | ||
| result = result.replace(regex, s.value); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent regex injection when constructing RegExp from variables.
The g.key and s.key values are interpolated directly into the RegExp constructor. If a key contains special regex characters, it could result in a broken regex or potentially a Regular Expression Denial of Service (ReDoS) vulnerability.
Consider escaping the keys before passing them to the constructor, or use a single static regex with a replacement callback function.
🛡️ Proposed fix (using static regex patterns)
Using a static regex with a callback is both safer and more efficient:
- if (globals) {
- globals.forEach((g) => {
- const regex = new RegExp(`\\{\\{\\s*global\\.${g.key}\\s*\\}\\}`, "g");
- result = result.replace(regex, g.value);
- });
- }
-
- if (secrets) {
- secrets.forEach((s) => {
- const regex = new RegExp(`\\{\\{\\s*secret\\.${s.key}\\s*\\}\\}`, "g");
- result = result.replace(regex, s.value);
- });
- }
+ if (globals) {
+ result = result.replace(/\{\{\s*global\.([^}]+)\s*\}\}/g, (match, k) => {
+ const g = globals.find((item) => item.key === k.trim());
+ return g ? g.value : match;
+ });
+ }
+
+ if (secrets) {
+ result = result.replace(/\{\{\s*secret\.([^}]+)\s*\}\}/g, (match, k) => {
+ const s = secrets.find((item) => item.key === k.trim());
+ return s ? s.value : match;
+ });
+ }📝 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.
| globals.forEach((g) => { | |
| const regex = new RegExp(`\\{\\{\\s*global\\.${g.key}\\s*\\}\\}`, "g"); | |
| result = result.replace(regex, g.value); | |
| }); | |
| } | |
| if (secrets) { | |
| secrets.forEach((s) => { | |
| const regex = new RegExp(`\\{\\{\\s*secret\\.${s.key}\\s*\\}\\}`, "g"); | |
| result = result.replace(regex, s.value); | |
| }); | |
| if (globals) { | |
| result = result.replace(/\{\{\s*global\.([^}]+)\s*\}\}/g, (match, k) => { | |
| const g = globals.find((item) => item.key === k.trim()); | |
| return g ? g.value : match; | |
| }); | |
| } | |
| if (secrets) { | |
| result = result.replace(/\{\{\s*secret\.([^}]+)\s*\}\}/g, (match, k) => { | |
| const s = secrets.find((item) => item.key === k.trim()); | |
| return s ? s.value : match; | |
| }); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 56-56: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\{\\{\\s*global\\.${g.key}\\s*\\}\\}, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 63-63: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\{\\{\\s*secret\\.${s.key}\\s*\\}\\}, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/flow/runFlow.ts` around lines 56 - 66, Escape g.key and s.key
before interpolating them into the RegExp constructors in the global and secret
replacement loops, or replace both dynamic patterns with static regexes and
callback-based lookups. Ensure keys containing regex metacharacters are matched
literally while preserving the existing placeholder replacement behavior.
Source: Linters/SAST tools
1 participant
Implemented a fully featured, client-side Global Variables and Secrets manager for the AI agent workflow builder. Allows users to save env-like variables securely in localStorage, resolve them as
{{global.KEY}}or{{secret.KEY}}placeholders inside node configurations, and export them as dynamic objects in generated Python and JS scripts. Verified with Vitest tests and Playwright screenshots.PR created automatically by Jules for task 12275737408163780391 started by @Jacobcdsmith
Summary by CodeRabbit
New Features
{{global.key}}and{{secret.key}}.Tests