Enhancement of Globals/Secrets Management and Run Logs Drawer - #12
Merged
Jacobcdsmith merged 1 commit intoJul 26, 2026
Merged
Conversation
- Added sidebar search, bulk clear, export-to-clipboard, and schema-validated JSON import (merge/replace modes) to Globals & Secrets Manager. - Added full-text search filtering, mass expand/collapse details, clear logs, and JSON logs download to Execution Run Log drawer. - Added comprehensive unit tests in `globalsAndLogs.test.tsx` verifying all new functionality. - Fully verified all changes visually and with 16 green unit tests.
|
👋 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. |
Contributor
📝 WalkthroughWalkthroughGlobalsManager gains search, bulk clear, clipboard export, validated JSON import, and related UI tests. Index adds run-log filtering, snapshot expansion controls, clearing, export, and utility toolbar rendering. ChangesGlobals and secrets management
Run-log utilities
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)Globals import flowsequenceDiagram
participant User
participant ImportModal
participant GlobalsManager
participant ChangeCallbacks
User->>ImportModal: enter JSON and choose import mode
ImportModal->>GlobalsManager: submit importText
GlobalsManager->>ChangeCallbacks: update globals and secrets
GlobalsManager->>ImportModal: close modal or display importError
Run-log filtering and snapshotssequenceDiagram
participant User
participant LogDrawer
participant Index
User->>LogDrawer: enter search query
LogDrawer->>Index: update logsSearchQuery
Index->>LogDrawer: render filteredLogs
User->>LogDrawer: toggle snapshot expansion
LogDrawer->>Index: update expandedLogSnapshots
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
frontend/src/flow/GlobalsManager.tsx (4)
285-295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an accessible name to the search input.
Placeholder text isn't a reliable accessible name. An
aria-labelkeeps the control announced correctly by screen readers.♿ Proposed tweak
<input type="text" + aria-label="Search globals and secrets" value={searchQuery} onChange={(e) => setSearchQuery(e.target.value)} placeholder="Search keys, values..."🤖 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 285 - 295, Add an explicit accessible name to the search input in the GlobalsManager component by adding an aria-label that describes its purpose, while preserving the existing searchQuery value, change handler, placeholder, and styling.
134-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
toastinstead ofalertfor consistency.The rest of the app signals results through
toast(e.g.handleExportLogsinfrontend/src/pages/Index.tsxusestoast.error/toast.success). Blockingalert()here is a UX outlier;confirm()for the destructive action is reasonable to keep.🤖 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 134 - 144, Update handleClearAll to replace the blocking alert notification for an empty items list with the app’s established toast mechanism, using the appropriate error or informational toast while preserving the early return. Keep the existing confirm prompt and clearing behavior unchanged.
208-210: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilently dropped entries are invisible to the user.
Entries failing the shape check are skipped without any signal; the user only sees an error when every entry is invalid. Tracking a skipped count and surfacing it in the success message would make partial imports diagnosable.
🤖 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 208 - 210, Update the import logic around the importedGlobals/importedSecrets validation to count entries rejected by the shape check, while preserving the existing all-invalid error behavior. Include the skipped-entry count in the successful import message when any entries were dropped so partial imports are visible to the user.
522-534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport modal isn't dismissible via keyboard.
The overlay has no Escape handler and no initial focus, so keyboard users must tab to the
Closebutton. Worth wiringonKeyDown/useEffectfor Escape and autofocusing the textarea.🤖 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 522 - 534, Make the import modal keyboard-dismissible by handling Escape in the modal’s keyboard event flow and closing it through the existing cleanup logic. Add initial focus to the import textarea when the modal opens, using the component’s existing state and lifecycle patterns; update the relevant modal/textarea elements in GlobalsManager.frontend/src/pages/Index.tsx (2)
653-688: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
useCallbackfor consistency.Every other handler in this component is memoized; these four are plain functions. Purely stylistic here since none are passed to memoized children.
🤖 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/pages/Index.tsx` around lines 653 - 688, Memoize the four log handlers—handleLogsExpandAll, handleLogsCollapseAll, handleClearLogs, and handleExportLogs—with useCallback, preserving their existing behavior and adding dependency arrays for the state, toast, and other values they reference.
690-705: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a "no matching logs" empty state.
When the query matches nothing,
filteredLogsis empty and the list area renders nothing below the utilities panel, which reads as a broken drawer rather than an empty filter result.♻️ Suggested addition after the log list
+ {runLogs && runLogs.length > 0 && filteredLogs.length === 0 && ( + <div className="font-mono text-[10px] text-[hsl(var(--ink-faint))] uppercase tracking-[0.15em]"> + no logs match "{logsSearchQuery}" + </div> + )}🤖 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/pages/Index.tsx` around lines 690 - 705, Update the run logs list rendering after the utilities panel to show a clear “no matching logs” empty state when logsSearchQuery is non-empty and filteredLogs contains no entries. Preserve the existing list rendering when matches exist and the existing behavior for an empty query or unavailable runLogs.
🤖 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/GlobalsManager.tsx`:
- Around line 34-47: Update the filteredItems search in GlobalsManager so
item.value is considered only when item.type === "global", while retaining key
and type matching for all items. Initialize selectedId from items[0]?.id instead
of filteredItems[0]?.id to reflect useState’s first-render behavior.
- Around line 146-154: Update handleExport to guard navigator.clipboard and
handle unavailable or synchronous write failures without uncaught errors, while
preserving the existing success/failure feedback. Before exporting secrets in
plaintext, require explicit user confirmation or provide a secrets-excluded
export option; ensure the clipboard payload matches that choice.
- Around line 212-238: Update the merge branch around mergedGlobalsMap and
mergedSecretsMap to enforce key uniqueness across both collections, not just
within each type. Apply the imported-type-wins rule: when an imported global or
secret key matches an existing or imported entry of the other type, remove the
losing entry before calling onGlobalsChange and onSecretsChange; preserve the
existing same-type update behavior and selection handling.
- Around line 163-210: Update the import validation in the flat-array and nested
globals/secrets branches to require keys accepted by validateKey, skipping or
reporting invalid, empty, malformed, or whitespace-containing keys. Replace
unconditional reuse of item.id, g.id, and s.id with fresh cryptoId values, or
retain supplied IDs only after verifying they do not collide with existing
globals, secrets, or other imported entries.
In `@frontend/src/pages/Index.tsx`:
- Around line 1287-1293: Add an accessible name to the logs search input
associated with logsSearchQuery by setting its aria-label to “Filter run logs”;
keep the existing value, change handler, placeholder, and styling unchanged.
- Around line 674-688: The handleExportLogs function should export filteredLogs
instead of runLogs, while retaining the existing empty-result guard and download
flow. Before JSON serialization, remove or redact metadata and snapshot fields
derived from secret.* values, including secrets interpolated into outputs, so
the downloaded file contains no secret-derived data.
- Around line 653-672: Reset logsSearchQuery and expandedLogSnapshots whenever
logs are cleared or a new run starts. Update handleClearLogs alongside its
existing runLogs reset, and update runFlowAction’s run initialization alongside
the runLogs reset so both paths remove stale filtering and expansion state
before displaying fresh logs.
In `@frontend/src/test/globalsAndLogs.test.tsx`:
- Around line 215-225: Update the malformed-JSON assertion in the import-submit
test to avoid matching the engine-specific JSON.parse error text; assert only
the stable warning marker, or normalize parse failures in handleImportSubmit to
a fixed message and assert that. Add coverage for valid JSON with no recognized
globals or secrets and for the handleClearAll behavior.
- Around line 59-96: Move the shared navigator.clipboard and window.alert
mocking from individual tests into beforeEach, and restore both globals in
afterEach so cleanup runs even when assertions fail. Import afterEach from
vitest, remove each test’s originalAlert save/restore logic, and apply the setup
consistently to the affected GlobalsManager tests.
---
Nitpick comments:
In `@frontend/src/flow/GlobalsManager.tsx`:
- Around line 285-295: Add an explicit accessible name to the search input in
the GlobalsManager component by adding an aria-label that describes its purpose,
while preserving the existing searchQuery value, change handler, placeholder,
and styling.
- Around line 134-144: Update handleClearAll to replace the blocking alert
notification for an empty items list with the app’s established toast mechanism,
using the appropriate error or informational toast while preserving the early
return. Keep the existing confirm prompt and clearing behavior unchanged.
- Around line 208-210: Update the import logic around the
importedGlobals/importedSecrets validation to count entries rejected by the
shape check, while preserving the existing all-invalid error behavior. Include
the skipped-entry count in the successful import message when any entries were
dropped so partial imports are visible to the user.
- Around line 522-534: Make the import modal keyboard-dismissible by handling
Escape in the modal’s keyboard event flow and closing it through the existing
cleanup logic. Add initial focus to the import textarea when the modal opens,
using the component’s existing state and lifecycle patterns; update the relevant
modal/textarea elements in GlobalsManager.
In `@frontend/src/pages/Index.tsx`:
- Around line 653-688: Memoize the four log handlers—handleLogsExpandAll,
handleLogsCollapseAll, handleClearLogs, and handleExportLogs—with useCallback,
preserving their existing behavior and adding dependency arrays for the state,
toast, and other values they reference.
- Around line 690-705: Update the run logs list rendering after the utilities
panel to show a clear “no matching logs” empty state when logsSearchQuery is
non-empty and filteredLogs contains no entries. Preserve the existing list
rendering when matches exist and the existing behavior for an empty query or
unavailable runLogs.
🪄 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 Plus
Run ID: d14c0980-2a1f-4886-97f7-c7f9126a6924
⛔ Files ignored due to path filters (1)
server.logis excluded by!**/*.log
📒 Files selected for processing (3)
frontend/src/flow/GlobalsManager.tsxfrontend/src/pages/Index.tsxfrontend/src/test/globalsAndLogs.test.tsx
Comment on lines
+34
to
47
| const filteredItems = useMemo(() => { | ||
| const query = searchQuery.trim().toLowerCase(); | ||
| if (!query) return items; | ||
| return items.filter( | ||
| (item) => | ||
| item.key.toLowerCase().includes(query) || | ||
| item.value.toLowerCase().includes(query) || | ||
| item.type.includes(query) | ||
| ); | ||
| }, [items, searchQuery]); | ||
|
|
||
| const [selectedId, setSelectedId] = useState<string | null>( | ||
| items[0]?.id ?? null | ||
| filteredItems[0]?.id ?? null | ||
| ); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Search matches secret values in plaintext, and filteredItems[0] in the initializer is misleading.
Two points on this block:
item.value.toLowerCase().includes(query)also matches secret values. Since secrets are otherwise masked behind the password input and theshowtoggle, substring search lets a value be confirmed without revealing it deliberately. Consider restricting value matching totype === "global".useState(filteredItems[0]?.id ?? null)only evaluates on first render, whensearchQueryis""andfilteredItems === items. Usingitems[0]?.idstates the intent more accurately.
🔐 Proposed adjustment
return items.filter(
(item) =>
item.key.toLowerCase().includes(query) ||
- item.value.toLowerCase().includes(query) ||
+ (item.type === "global" && item.value.toLowerCase().includes(query)) ||
item.type.includes(query)
);
}, [items, searchQuery]);
const [selectedId, setSelectedId] = useState<string | null>(
- filteredItems[0]?.id ?? null
+ items[0]?.id ?? null
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const filteredItems = useMemo(() => { | |
| const query = searchQuery.trim().toLowerCase(); | |
| if (!query) return items; | |
| return items.filter( | |
| (item) => | |
| item.key.toLowerCase().includes(query) || | |
| item.value.toLowerCase().includes(query) || | |
| item.type.includes(query) | |
| ); | |
| }, [items, searchQuery]); | |
| const [selectedId, setSelectedId] = useState<string | null>( | |
| items[0]?.id ?? null | |
| filteredItems[0]?.id ?? null | |
| ); | |
| const filteredItems = useMemo(() => { | |
| const query = searchQuery.trim().toLowerCase(); | |
| if (!query) return items; | |
| return items.filter( | |
| (item) => | |
| item.key.toLowerCase().includes(query) || | |
| (item.type === "global" && item.value.toLowerCase().includes(query)) || | |
| item.type.includes(query) | |
| ); | |
| }, [items, searchQuery]); | |
| const [selectedId, setSelectedId] = useState<string | null>( | |
| items[0]?.id ?? null | |
| ); |
🤖 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 34 - 47, Update the
filteredItems search in GlobalsManager so item.value is considered only when
item.type === "global", while retaining key and type matching for all items.
Initialize selectedId from items[0]?.id instead of filteredItems[0]?.id to
reflect useState’s first-render behavior.
Comment on lines
+146
to
+154
| const handleExport = () => { | ||
| const data = { | ||
| globals, | ||
| secrets, | ||
| }; | ||
| navigator.clipboard.writeText(JSON.stringify(data, null, 2)) | ||
| .then(() => alert("Environment configuration copied to clipboard!")) | ||
| .catch((err) => alert(`Failed to copy to clipboard: ${err}`)); | ||
| }; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Guard navigator.clipboard and warn before copying secrets in plaintext.
navigator.clipboard is undefined in non-secure contexts (and in some embedded webviews), so writeText throws synchronously — the .catch never runs and the click produces an uncaught TypeError. Also, this copies every secret value in cleartext to the system clipboard with no confirmation; at minimum confirm, or offer a secrets-excluded export.
🛡️ Proposed fix
const handleExport = () => {
+ if (!navigator.clipboard?.writeText) {
+ alert("Clipboard access is unavailable in this context.");
+ return;
+ }
+ if (secrets.length > 0 && !confirm("This copies all secret values in plaintext to your clipboard. Continue?")) {
+ return;
+ }
const data = {
globals,
secrets,
};
navigator.clipboard.writeText(JSON.stringify(data, null, 2))
.then(() => alert("Environment configuration copied to clipboard!"))
.catch((err) => alert(`Failed to copy to clipboard: ${err}`));
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleExport = () => { | |
| const data = { | |
| globals, | |
| secrets, | |
| }; | |
| navigator.clipboard.writeText(JSON.stringify(data, null, 2)) | |
| .then(() => alert("Environment configuration copied to clipboard!")) | |
| .catch((err) => alert(`Failed to copy to clipboard: ${err}`)); | |
| }; | |
| const handleExport = () => { | |
| if (!navigator.clipboard?.writeText) { | |
| alert("Clipboard access is unavailable in this context."); | |
| return; | |
| } | |
| if (secrets.length > 0 && !confirm("This copies all secret values in plaintext to your clipboard. Continue?")) { | |
| return; | |
| } | |
| const data = { | |
| globals, | |
| secrets, | |
| }; | |
| navigator.clipboard.writeText(JSON.stringify(data, null, 2)) | |
| .then(() => alert("Environment configuration copied to clipboard!")) | |
| .catch((err) => alert(`Failed to copy to clipboard: ${err}`)); | |
| }; |
🤖 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 146 - 154, Update
handleExport to guard navigator.clipboard and handle unavailable or synchronous
write failures without uncaught errors, while preserving the existing
success/failure feedback. Before exporting secrets in plaintext, require
explicit user confirmation or provide a secrets-excluded export option; ensure
the clipboard payload matches that choice.
Comment on lines
+163
to
+210
| if (Array.isArray(parsed)) { | ||
| // Flat array format | ||
| parsed.forEach((item: any) => { | ||
| if (item && typeof item === 'object' && typeof item.key === 'string' && typeof item.value === 'string') { | ||
| const type = item.type === 'secret' ? 'secret' : 'global'; | ||
| const newItem = { | ||
| id: item.id || cryptoId(), | ||
| key: item.key.trim(), | ||
| value: item.value, | ||
| }; | ||
| if (type === 'global') { | ||
| importedGlobals.push(newItem); | ||
| } else { | ||
| importedSecrets.push(newItem); | ||
| } | ||
| } | ||
| }); | ||
| } else if (parsed && typeof parsed === 'object') { | ||
| // Nested format { globals: [...], secrets: [...] } | ||
| if (Array.isArray(parsed.globals)) { | ||
| parsed.globals.forEach((g: any) => { | ||
| if (g && typeof g === 'object' && typeof g.key === 'string' && typeof g.value === 'string') { | ||
| importedGlobals.push({ | ||
| id: g.id || cryptoId(), | ||
| key: g.key.trim(), | ||
| value: g.value, | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| if (Array.isArray(parsed.secrets)) { | ||
| parsed.secrets.forEach((s: any) => { | ||
| if (s && typeof s === 'object' && typeof s.key === 'string' && typeof s.value === 'string') { | ||
| importedSecrets.push({ | ||
| id: s.id || cryptoId(), | ||
| key: s.key.trim(), | ||
| value: s.value, | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| } else { | ||
| throw new Error("Invalid format. Expected a list or { globals, secrets } object."); | ||
| } | ||
|
|
||
| if (importedGlobals.length === 0 && importedSecrets.length === 0) { | ||
| throw new Error("No valid global variables or secrets found in the JSON."); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Imported entries bypass key validation and can reuse existing ids.
Two gaps in the validation loop:
- Only
typeof key === 'string'is checked, so"","1BAD-KEY", or"has spaces"are imported and merely rendered with a ⚠ afterwards. ReusevalidateKey's identifier regex here and skip/report offenders. id: item.id || cryptoId()trusts caller-supplied ids. Merge dedupes by key, so an imported item carrying an id already present inglobals/secretsyields duplicate ids — duplicate React keys in the sidebar anditems.find((i) => i.id === selectedId)resolving to the wrong record. Safer to always mint a fresh id, or only keepitem.idwhen it doesn't collide.
Same applies to the nested parsed.globals / parsed.secrets branches.
🐛 Proposed fix (flat-array branch shown; mirror for nested branch)
+const KEY_RE = /^[a-zA-Z_][a-zA-Z0-9_]*$/;
+
if (Array.isArray(parsed)) {
// Flat array format
parsed.forEach((item: any) => {
- if (item && typeof item === 'object' && typeof item.key === 'string' && typeof item.value === 'string') {
+ if (
+ item && typeof item === 'object' &&
+ typeof item.key === 'string' && KEY_RE.test(item.key.trim()) &&
+ typeof item.value === 'string'
+ ) {
const type = item.type === 'secret' ? 'secret' : 'global';
const newItem = {
- id: item.id || cryptoId(),
+ id: cryptoId(),
key: item.key.trim(),
value: item.value,
};🤖 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 163 - 210, Update the
import validation in the flat-array and nested globals/secrets branches to
require keys accepted by validateKey, skipping or reporting invalid, empty,
malformed, or whitespace-containing keys. Replace unconditional reuse of
item.id, g.id, and s.id with fresh cryptoId values, or retain supplied IDs only
after verifying they do not collide with existing globals, secrets, or other
imported entries.
Comment on lines
+212
to
+238
| if (mode === "replace") { | ||
| onGlobalsChange(importedGlobals); | ||
| onSecretsChange(importedSecrets); | ||
| const remaining = [...importedGlobals.map((g) => g.id), ...importedSecrets.map((s) => s.id)]; | ||
| setSelectedId(remaining[0] ?? null); | ||
| } else { | ||
| // Merge mode: keys must be unique. Update existing, append new. | ||
| const mergedGlobalsMap = new Map<string, GlobalVar>(); | ||
| globals.forEach((g) => mergedGlobalsMap.set(g.key.toLowerCase(), g)); | ||
| importedGlobals.forEach((ig) => { | ||
| mergedGlobalsMap.set(ig.key.toLowerCase(), ig); | ||
| }); | ||
|
|
||
| const mergedSecretsMap = new Map<string, SecretVar>(); | ||
| secrets.forEach((s) => mergedSecretsMap.set(s.key.toLowerCase(), s)); | ||
| importedSecrets.forEach((is) => { | ||
| mergedSecretsMap.set(is.key.toLowerCase(), is); | ||
| }); | ||
|
|
||
| const finalGlobals = Array.from(mergedGlobalsMap.values()); | ||
| const finalSecrets = Array.from(mergedSecretsMap.values()); | ||
|
|
||
| onGlobalsChange(finalGlobals); | ||
| onSecretsChange(finalSecrets); | ||
| const remaining = [...finalGlobals.map((g) => g.id), ...finalSecrets.map((s) => s.id)]; | ||
| setSelectedId(remaining[0] ?? null); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Merge keeps cross-type key collisions, violating the uniqueness invariant.
mergedGlobalsMap and mergedSecretsMap are independent, so importing a secret named BASE_API while a global BASE_API exists leaves both in state. validateKey (Line 66) enforces uniqueness across globals and secrets, so the merged result is immediately flagged invalid and {{global.BASE_API}} / {{secret.BASE_API}} resolution becomes ambiguous. Decide a precedence rule (imported type wins, or reject the import) and drop the loser from the other collection.
🐛 Proposed fix — imported type wins on cross-type collision
const finalGlobals = Array.from(mergedGlobalsMap.values());
const finalSecrets = Array.from(mergedSecretsMap.values());
+
+ // A key may exist in only one collection; the imported type wins.
+ const importedGlobalKeys = new Set(importedGlobals.map((g) => g.key.toLowerCase()));
+ const importedSecretKeys = new Set(importedSecrets.map((s) => s.key.toLowerCase()));
+ const dedupedGlobals = finalGlobals.filter((g) => !importedSecretKeys.has(g.key.toLowerCase()));
+ const dedupedSecrets = finalSecrets.filter((s) => !importedGlobalKeys.has(s.key.toLowerCase()));
- onGlobalsChange(finalGlobals);
- onSecretsChange(finalSecrets);
- const remaining = [...finalGlobals.map((g) => g.id), ...finalSecrets.map((s) => s.id)];
+ onGlobalsChange(dedupedGlobals);
+ onSecretsChange(dedupedSecrets);
+ const remaining = [...dedupedGlobals.map((g) => g.id), ...dedupedSecrets.map((s) => s.id)];
setSelectedId(remaining[0] ?? null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (mode === "replace") { | |
| onGlobalsChange(importedGlobals); | |
| onSecretsChange(importedSecrets); | |
| const remaining = [...importedGlobals.map((g) => g.id), ...importedSecrets.map((s) => s.id)]; | |
| setSelectedId(remaining[0] ?? null); | |
| } else { | |
| // Merge mode: keys must be unique. Update existing, append new. | |
| const mergedGlobalsMap = new Map<string, GlobalVar>(); | |
| globals.forEach((g) => mergedGlobalsMap.set(g.key.toLowerCase(), g)); | |
| importedGlobals.forEach((ig) => { | |
| mergedGlobalsMap.set(ig.key.toLowerCase(), ig); | |
| }); | |
| const mergedSecretsMap = new Map<string, SecretVar>(); | |
| secrets.forEach((s) => mergedSecretsMap.set(s.key.toLowerCase(), s)); | |
| importedSecrets.forEach((is) => { | |
| mergedSecretsMap.set(is.key.toLowerCase(), is); | |
| }); | |
| const finalGlobals = Array.from(mergedGlobalsMap.values()); | |
| const finalSecrets = Array.from(mergedSecretsMap.values()); | |
| onGlobalsChange(finalGlobals); | |
| onSecretsChange(finalSecrets); | |
| const remaining = [...finalGlobals.map((g) => g.id), ...finalSecrets.map((s) => s.id)]; | |
| setSelectedId(remaining[0] ?? null); | |
| } | |
| if (mode === "replace") { | |
| onGlobalsChange(importedGlobals); | |
| onSecretsChange(importedSecrets); | |
| const remaining = [...importedGlobals.map((g) => g.id), ...importedSecrets.map((s) => s.id)]; | |
| setSelectedId(remaining[0] ?? null); | |
| } else { | |
| // Merge mode: keys must be unique. Update existing, append new. | |
| const mergedGlobalsMap = new Map<string, GlobalVar>(); | |
| globals.forEach((g) => mergedGlobalsMap.set(g.key.toLowerCase(), g)); | |
| importedGlobals.forEach((ig) => { | |
| mergedGlobalsMap.set(ig.key.toLowerCase(), ig); | |
| }); | |
| const mergedSecretsMap = new Map<string, SecretVar>(); | |
| secrets.forEach((s) => mergedSecretsMap.set(s.key.toLowerCase(), s)); | |
| importedSecrets.forEach((is) => { | |
| mergedSecretsMap.set(is.key.toLowerCase(), is); | |
| }); | |
| const finalGlobals = Array.from(mergedGlobalsMap.values()); | |
| const finalSecrets = Array.from(mergedSecretsMap.values()); | |
| // A key may exist in only one collection; the imported type wins. | |
| const importedGlobalKeys = new Set(importedGlobals.map((g) => g.key.toLowerCase())); | |
| const importedSecretKeys = new Set(importedSecrets.map((s) => s.key.toLowerCase())); | |
| const dedupedGlobals = finalGlobals.filter((g) => !importedSecretKeys.has(g.key.toLowerCase())); | |
| const dedupedSecrets = finalSecrets.filter((s) => !importedGlobalKeys.has(s.key.toLowerCase())); | |
| onGlobalsChange(dedupedGlobals); | |
| onSecretsChange(dedupedSecrets); | |
| const remaining = [...dedupedGlobals.map((g) => g.id), ...dedupedSecrets.map((s) => s.id)]; | |
| setSelectedId(remaining[0] ?? null); | |
| } |
🤖 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 212 - 238, Update the
merge branch around mergedGlobalsMap and mergedSecretsMap to enforce key
uniqueness across both collections, not just within each type. Apply the
imported-type-wins rule: when an imported global or secret key matches an
existing or imported entry of the other type, remove the losing entry before
calling onGlobalsChange and onSecretsChange; preserve the existing same-type
update behavior and selection handling.
Comment on lines
+653
to
+672
| // Expand / Collapse and Clear logs actions | ||
| const handleLogsExpandAll = () => { | ||
| if (!runLogs) return; | ||
| const patch: Record<string, boolean> = {}; | ||
| runLogs.forEach((l) => { | ||
| if (l.stateSnapshot) { | ||
| patch[`${l.step}-${l.nodeId}`] = true; | ||
| } | ||
| }); | ||
| setExpandedLogSnapshots(patch); | ||
| }; | ||
|
|
||
| const handleLogsCollapseAll = () => { | ||
| setExpandedLogSnapshots({}); | ||
| }; | ||
|
|
||
| const handleClearLogs = () => { | ||
| setRunLogs(null); | ||
| setExpandedLogSnapshots({}); | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset logsSearchQuery on clear/rerun; stale filter hides fresh logs.
handleClearLogs clears the snapshot map but leaves logsSearchQuery set. Same for runFlowAction (line 583), which resets runLogs to [] but leaves both logsSearchQuery and expandedLogSnapshots from the previous run. Net effect: after a rerun with a leftover query, the utilities panel renders while the log list looks empty, and previously expanded step-nodeId keys re-expand snapshots in the new run.
🔧 Proposed fix
const handleClearLogs = () => {
setRunLogs(null);
setExpandedLogSnapshots({});
+ setLogsSearchQuery("");
};And in runFlowAction where the run is initialized:
setRunLogs([]);
+ setExpandedLogSnapshots({});
+ setLogsSearchQuery("");🤖 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/pages/Index.tsx` around lines 653 - 672, Reset logsSearchQuery
and expandedLogSnapshots whenever logs are cleared or a new run starts. Update
handleClearLogs alongside its existing runLogs reset, and update runFlowAction’s
run initialization alongside the runLogs reset so both paths remove stale
filtering and expansion state before displaying fresh logs.
Comment on lines
+674
to
+688
| const handleExportLogs = () => { | ||
| if (!runLogs || runLogs.length === 0) { | ||
| toast.error("No logs to export"); | ||
| return; | ||
| } | ||
| const data = JSON.stringify(runLogs, null, 2); | ||
| const blob = new Blob([data], { type: "application/json;charset=utf-8" }); | ||
| const url = URL.createObjectURL(blob); | ||
| const a = document.createElement("a"); | ||
| a.href = url; | ||
| a.download = "agent_flow_run_logs.json"; | ||
| a.click(); | ||
| URL.revokeObjectURL(url); | ||
| toast.success("Logs downloaded"); | ||
| }; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how stateSnapshot/output are populated and whether secrets are redacted.
fd -t f 'runFlow.*' frontend/src --exec ast-grep outline {} --items all
rg -nP -C4 '\b(stateSnapshot|secrets)\b' frontend/src/flow/runFlow.tsRepository: Jacobcdsmith/agent-flow-canvas
Length of output: 3346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Index.tsx relevant sections:"
sed -n '620,710p' frontend/src/pages/Index.tsx
echo
echo "runFlow output/stateSnapshot construction:"
sed -n '66,220p' frontend/src/flow/runFlow.ts
echo
echo "Search for redact/omit/secret masking / logs in frontend:"
rg -n -C3 '\b(redact|omit|stateSnapshot|secrets)\b|mask|hide|sensitive' frontend/src/pages frontend/src/flow || trueRepository: Jacobcdsmith/agent-flow-canvas
Length of output: 33347
Export filtered logs and redact secret-derived snapshot fields.
handleExportLogs downloads runLogs while a query filters the visible table to filteredLogs, so the file can contain entries the user is not seeing. Also, runFlow interpolates secret.* values into outputs and snapshots the state before any log-level redaction, so secrets can be written into the downloaded JSON. Export filteredLogs with the same empty-guard, and remove/redact secret-derived metadata before serialization.
🤖 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/pages/Index.tsx` around lines 674 - 688, The handleExportLogs
function should export filteredLogs instead of runLogs, while retaining the
existing empty-result guard and download flow. Before JSON serialization, remove
or redact metadata and snapshot fields derived from secret.* values, including
secrets interpolated into outputs, so the downloaded file contains no
secret-derived data.
Comment on lines
+1287
to
+1293
| <input | ||
| type="text" | ||
| value={logsSearchQuery} | ||
| onChange={(e) => setLogsSearchQuery(e.target.value)} | ||
| placeholder="Filter logs by name, output, error..." | ||
| className="w-full bg-transparent border border-dashed border-[hsl(var(--ink-faint))] focus:border-[hsl(var(--ink))] outline-none py-1 px-2 font-mono text-[10px] text-[hsl(var(--ink))] mb-1" | ||
| /> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Filter input has no accessible name.
Placeholder-only labelling isn't exposed reliably to assistive tech. Add aria-label="Filter run logs" (or an associated visually-hidden label).
🤖 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/pages/Index.tsx` around lines 1287 - 1293, Add an accessible
name to the logs search input associated with logsSearchQuery by setting its
aria-label to “Filter run logs”; keep the existing value, change handler,
placeholder, and styling unchanged.
Comment on lines
+59
to
+96
| // Mock clipboard API via Object.defineProperty | ||
| const writeTextMock = vi.fn().mockImplementation(() => Promise.resolve()); | ||
| Object.defineProperty(navigator, "clipboard", { | ||
| value: { | ||
| writeText: writeTextMock, | ||
| }, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| // Mock window.alert | ||
| const originalAlert = window.alert; | ||
| window.alert = vi.fn(); | ||
|
|
||
| render( | ||
| <GlobalsManager | ||
| globals={mockGlobals} | ||
| secrets={mockSecrets} | ||
| onGlobalsChange={onGlobalsChange} | ||
| onSecretsChange={onSecretsChange} | ||
| onClose={onClose} | ||
| /> | ||
| ); | ||
|
|
||
| const exportBtn = screen.getByText("Export"); | ||
| fireEvent.click(exportBtn); | ||
|
|
||
| expect(writeTextMock).toHaveBeenCalledTimes(1); | ||
| const exportedData = JSON.parse(writeTextMock.mock.calls[0][0]); | ||
| expect(exportedData.globals).toEqual(mockGlobals); | ||
| expect(exportedData.secrets).toEqual(mockSecrets); | ||
|
|
||
| await waitFor(() => { | ||
| expect(window.alert).toHaveBeenCalledWith("Environment configuration copied to clipboard!"); | ||
| }); | ||
|
|
||
| window.alert = originalAlert; | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Global mutations leak between tests.
navigator.clipboard is redefined and never restored, and window.alert is restored only on the happy path — any failing assertion above Line 95 leaves both stubs installed for every subsequent test in the run. Move the setup/teardown into beforeEach/afterEach (or vi.stubGlobal, which vi.unstubAllGlobals() reverses) so failures can't cascade.
🧪 Proposed restructuring
describe("GlobalsManager - Enhanced Features", () => {
+ const originalClipboard = navigator.clipboard;
+ const originalAlert = window.alert;
+
+ afterEach(() => {
+ Object.defineProperty(navigator, "clipboard", {
+ value: originalClipboard,
+ writable: true,
+ configurable: true,
+ });
+ window.alert = originalAlert;
+ vi.restoreAllMocks();
+ });Then drop the per-test originalAlert save/restore lines (Lines 70-71, 95, 114-115, 149, 168-169, 194). Remember to import afterEach from vitest.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Mock clipboard API via Object.defineProperty | |
| const writeTextMock = vi.fn().mockImplementation(() => Promise.resolve()); | |
| Object.defineProperty(navigator, "clipboard", { | |
| value: { | |
| writeText: writeTextMock, | |
| }, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| // Mock window.alert | |
| const originalAlert = window.alert; | |
| window.alert = vi.fn(); | |
| render( | |
| <GlobalsManager | |
| globals={mockGlobals} | |
| secrets={mockSecrets} | |
| onGlobalsChange={onGlobalsChange} | |
| onSecretsChange={onSecretsChange} | |
| onClose={onClose} | |
| /> | |
| ); | |
| const exportBtn = screen.getByText("Export"); | |
| fireEvent.click(exportBtn); | |
| expect(writeTextMock).toHaveBeenCalledTimes(1); | |
| const exportedData = JSON.parse(writeTextMock.mock.calls[0][0]); | |
| expect(exportedData.globals).toEqual(mockGlobals); | |
| expect(exportedData.secrets).toEqual(mockSecrets); | |
| await waitFor(() => { | |
| expect(window.alert).toHaveBeenCalledWith("Environment configuration copied to clipboard!"); | |
| }); | |
| window.alert = originalAlert; | |
| }); | |
| const originalClipboard = navigator.clipboard; | |
| const originalAlert = window.alert; | |
| afterEach(() => { | |
| Object.defineProperty(navigator, "clipboard", { | |
| value: originalClipboard, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| window.alert = originalAlert; | |
| vi.restoreAllMocks(); | |
| }); |
🤖 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/test/globalsAndLogs.test.tsx` around lines 59 - 96, Move the
shared navigator.clipboard and window.alert mocking from individual tests into
beforeEach, and restore both globals in afterEach so cleanup runs even when
assertions fail. Import afterEach from vitest, remove each test’s originalAlert
save/restore logic, and apply the setup consistently to the affected
GlobalsManager tests.
Comment on lines
+215
to
+225
| // Provide malformed JSON | ||
| const textarea = screen.getByPlaceholderText("Paste JSON content here..."); | ||
| fireEvent.change(textarea, { target: { value: "{ malformed: json" } }); | ||
|
|
||
| const mergeBtn = screen.getByText("Merge-Import"); | ||
| fireEvent.click(mergeBtn); | ||
|
|
||
| // Should render warning on error | ||
| expect(screen.getByText(/⚠ Expected/)).toBeInTheDocument(); | ||
| expect(onGlobalsChange).not.toHaveBeenCalled(); | ||
| expect(onSecretsChange).not.toHaveBeenCalled(); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
/⚠ Expected/ asserts on the JS engine's JSON.parse message.
importError here is whatever JSON.parse threw, and that wording is engine- and version-specific (V8 has changed it across Node releases; other engines phrase it differently). Assert on the ⚠ marker alone, or have handleImportSubmit normalize parse failures to a fixed message such as "Invalid JSON syntax" and assert that.
Also worth adding: a well-formed-but-wrong-shape payload (e.g. {"foo":1}) exercising the "No valid global variables or secrets found" path, and coverage for handleClearAll.
🧪 Proposed assertion change
- expect(screen.getByText(/⚠ Expected/)).toBeInTheDocument();
+ expect(screen.getByText(/^⚠/)).toBeInTheDocument();
expect(onGlobalsChange).not.toHaveBeenCalled();
expect(onSecretsChange).not.toHaveBeenCalled();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Provide malformed JSON | |
| const textarea = screen.getByPlaceholderText("Paste JSON content here..."); | |
| fireEvent.change(textarea, { target: { value: "{ malformed: json" } }); | |
| const mergeBtn = screen.getByText("Merge-Import"); | |
| fireEvent.click(mergeBtn); | |
| // Should render warning on error | |
| expect(screen.getByText(/⚠ Expected/)).toBeInTheDocument(); | |
| expect(onGlobalsChange).not.toHaveBeenCalled(); | |
| expect(onSecretsChange).not.toHaveBeenCalled(); | |
| // Provide malformed JSON | |
| const textarea = screen.getByPlaceholderText("Paste JSON content here..."); | |
| fireEvent.change(textarea, { target: { value: "{ malformed: json" } }); | |
| const mergeBtn = screen.getByText("Merge-Import"); | |
| fireEvent.click(mergeBtn); | |
| // Should render warning on error | |
| expect(screen.getByText(/^⚠/)).toBeInTheDocument(); | |
| expect(onGlobalsChange).not.toHaveBeenCalled(); | |
| expect(onSecretsChange).not.toHaveBeenCalled(); |
🤖 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/test/globalsAndLogs.test.tsx` around lines 215 - 225, Update the
malformed-JSON assertion in the import-submit test to avoid matching the
engine-specific JSON.parse error text; assert only the stable warning marker, or
normalize parse failures in handleImportSubmit to a fixed message and assert
that. Add coverage for valid JSON with no recognized globals or secrets and for
the handleClearAll behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This feature set introduces advanced environment variable search, backup/restore capabilities, and diagnostic run log controls to the visual canvas builder. All features are fully interactive, with zero placeholders, and covered by 16 passing automated tests.
PR created automatically by Jules for task 6780998033699171791 started by @Jacobcdsmith
Summary by CodeRabbit
New Features
Bug Fixes