Secrets list as dropdown from existing secrets and frontend warnings for deleted secrets #37
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
leon-schmid
force-pushed
the
ADM-369/Secrets-list-as-dropdown-from-existing-secrets
branch
from
933f15a
to
39f66a1
Compare
leon-schmid
changed the title
| return; | ||
| } | ||
| workflow.nodes.forEach((node, nodeIdx) => { | ||
| console.log("node", node); |
web/src/stores/workflow-store.ts
Outdated
| @@ -56,6 +56,7 @@ type WorkflowStoreState = TReactFlowGraph & { | |||
| webhookSecret: string | null; | |||
| lastDeletedEdges: TReactFlowEdge[]; | |||
| payloadCache: string; | |||
| missingSecretForNodes: string[]; | |||
There was a problem hiding this comment.
I don't think a mapping from index to missing secret is correct because the index of a node can change, e.g., if a node with a smaller index is deleted. You need to remember the nodes by their id and, therefore, I would suggest to use Set<string>.
General feedback on data structure here: If you want to use an array for flagging something, then don't use strings (waste of memory) instead use an array of boolean (still not optimal) or even better a bitset.
| {hasMissingSecret && ( | ||
| <Flex | ||
| className="absolute top-2 right-2" | ||
| justify="end" | ||
| align="start" | ||
| > | ||
| <Tooltip content="Missing secret"> | ||
| <Flex | ||
| className="bg-red-500 rounded-full w-6 h-6" | ||
| justify="center" | ||
| align="center" | ||
| > | ||
| <ExclamationTriangleIcon className="text-white w-4 h-4" /> | ||
| </Flex> | ||
| </Tooltip> | ||
| </Flex> | ||
| )} |
There was a problem hiding this comment.
Briefly tested it, this is also displayed for me if I click on "Create new workflow".
| const secretName = Object.values(node.secretsMapping)[0]; | ||
| if ( | ||
| secretName && | ||
| !encryptedSecrets.find((s) => s.secretId === secretName) |
There was a problem hiding this comment.
Minor: imo it is a bit cleaner to build a set of the secret ids before iterating over the nodes because then you can perform a lookup in constant time instead of always iterating here over all the encrypted secrets.
web/src/stores/workflow-store.ts
Outdated
| @@ -87,6 +88,9 @@ type WorkflowStoreState = TReactFlowGraph & { | |||
| deleteNodeByIdx: (nodeIdx: number) => void; | |||
| deleteControlByIdx: (controlIdx: number) => void; | |||
| duplicateNodeByIdx: (nodeIdx: number) => void; | |||
| addMissingSecretByIdx: (secret: string, nodeIdx: number) => void; | |||
| deleteMissingSecretByIdx: (idx: number) => void; | |||
| getMissingSecretByIdx: (idx: number) => string; | |||
There was a problem hiding this comment.
hasMissingSecret: (nodeId: string) => bool; is a cleaner interface imo
web/src/stores/workflow-store.ts
Outdated
| addMissingSecretByIdx: (secret: string, nodeIdx: number) => | ||
| set( | ||
| produce((draft) => { | ||
| draft.missingSecretForNodes[nodeIdx] = secret; |
There was a problem hiding this comment.
Interesting. Just learned that for out-of-bounds access the array automatically grows and initializes the new fields with undefined. Since you are relying on this, adding new nodes will always display an error regarding the not available secrets (see my start node comment)
| if (!acc.has(node.id)) { | ||
| acc.set(node.id, new Set()); | ||
| } | ||
| acc.get(node.id)?.add(secretIdx); |
There was a problem hiding this comment.
I just noticed that the secrets mapping in TEditorWorkflowActionNode is of type z.record(z.string(), z.string()) and not an array. So, the data structure should then be Map<string, Set<string>> while we add the secretsMapping to the set. So, you now need to iterate over the keys of node.secretsMapping instead of the values.
| @@ -29,5 +29,6 @@ export const useGetWorkflowApi = (workflowId: string, doApiCall: boolean) => { | |||
| queryKey: ["workflow", workflowId], | |||
| queryFn: () => getWorkflow({ workflowId }), | |||
| enabled: doApiCall, | |||
| gcTime: 0, | |||
There was a problem hiding this comment.
Can you add a comment here that this disables the cache?
There was a problem hiding this comment.
or instead: let's introduce a constant const DISABLE_CACHE = 0; and replace that with the magic number here.
web/src/stores/workflow-store.ts
Outdated
| if (missingSecrets) { | ||
| if (secretIdx !== undefined) { | ||
| return ( | ||
| missingSecrets.has(nodeId) && | ||
| missingSecrets.get(nodeId)!.has(secretIdx) | ||
| ); | ||
| } | ||
| return missingSecrets.has(nodeId); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Simplification suggestion:
| if (missingSecrets) { | |
| if (secretIdx !== undefined) { | |
| return ( | |
| missingSecrets.has(nodeId) && | |
| missingSecrets.get(nodeId)!.has(secretIdx) | |
| ); | |
| } | |
| return missingSecrets.has(nodeId); | |
| } | |
| return false; | |
| return !!(secretIdx === undefined ? missingSecrets?.has(nodeId) : (missingSecrets?.get(nodeId)?.has(secretIdx)); |
Note: secretIdx should be replaced with secretMappingPlaceholder (see comment above)
web/src/stores/workflow-store.ts
Outdated
| if (draft.missingSecretForNodes.has(nodeId)) { | ||
| draft.missingSecretForNodes.get(nodeId).delete(secretIdx); | ||
| if (draft.missingSecretForNodes.get(nodeId).size === 0) { | ||
| draft.missingSecretForNodes.delete(nodeId); | ||
| } | ||
| } |
There was a problem hiding this comment.
No need to perform lookup 3 times. I think you can just do:
| if (draft.missingSecretForNodes.has(nodeId)) { | |
| draft.missingSecretForNodes.get(nodeId).delete(secretIdx); | |
| if (draft.missingSecretForNodes.get(nodeId).size === 0) { | |
| draft.missingSecretForNodes.delete(nodeId); | |
| } | |
| } | |
| const secretPlaceholdersWithDeletedSecret = draft.missingSecretForNodes.get(nodeId); | |
| secretPlaceholdersWithDeletedSecret?.delete(secretIdx); | |
| if (secretPlaceholdersWithDeletedSecret?.size === 0) { | |
| draft.missingSecretForNodes.delete(nodeId); | |
| } |
web/src/stores/workflow-store.ts
Outdated
| @@ -106,6 +112,7 @@ export const useWorkflowStore = create<WorkflowStoreState>((set, get) => ({ | |||
| isActive: false, | |||
| nodes: [], | |||
| edges: [], | |||
| missingSecretForNodes: undefined, | |||
There was a problem hiding this comment.
I think this name is confusing because we are tracking nodes with deleted secrets.
| transform: "translate(10px, 24px)", | ||
| }} | ||
| > | ||
| <Flex className="absolute inset-0 bg-[var(--red-9)] rounded-full " /> |
There was a problem hiding this comment.
can you remove the whitespace at the end?
| <Flex className="absolute inset-0 bg-[var(--red-9)] rounded-full " /> | |
| <Flex className="absolute inset-0 bg-[var(--red-9)] rounded-full" /> |
| const onChangeSecretsMapping = ( | ||
| secretPlaceholder: string, | ||
| event: ChangeEvent<HTMLInputElement>, | ||
| value: string, | ||
| secretIdx: number, |
There was a problem hiding this comment.
please switch to secretPlaceholder (see comment below)
…n SaveWorkflowProvider
…usly selected secret is not available anymore
leon-schmid
force-pushed
the
ADM-369/Secrets-list-as-dropdown-from-existing-secrets
branch
from
a0b645c
to
3b631ff
Compare
web/src/stores/workflow-store.ts
Outdated
| @@ -87,6 +90,12 @@ type WorkflowStoreState = TReactFlowGraph & { | |||
| deleteNodeByIdx: (nodeIdx: number) => void; | |||
| deleteControlByIdx: (controlIdx: number) => void; | |||
| duplicateNodeByIdx: (nodeIdx: number) => void; | |||
| hasDeletedSecret: (nodeId: string, secretPlaceholer?: string) => boolean; | |||
| addDeletedSecrets: (missingSecrets: Map<string, Set<string>>) => void; | |||
There was a problem hiding this comment.
This a setter, so the better naming would be setDeletedSecrets. Also, the parameter name mismatches the function name. deletedSecrets would probably be more appropriate.
| .filter((node) => node.type === "action") | ||
| .reduce((acc, node) => { | ||
| const secretsMapping = node.secretsMapping; | ||
| const secretsForNode = Object.keys(secretsMapping); |
There was a problem hiding this comment.
secretsForNode --> secretPlaceholders
| useEffect(() => { | ||
| if (encryptedSecrets) { | ||
| setSecrets(encryptedSecrets); | ||
| return () => clear(); |
There was a problem hiding this comment.
Maybe we should rename clear from the secrets store to clearSecretsStore to be consistent with the workflow store naming. Also, makes it more clear here what's happening.
| <Flex | ||
| direction="row" | ||
| align="center" | ||
| className="text-red-500 font-medium mt-2" |
There was a problem hiding this comment.
If the component has props for doing what you want (text color, font weight), then please use this instead of using tailwindcss. You can just do <Text color="red" weight="medium"> for example.
There was a problem hiding this comment.
Also, <Flex> has props for margin top: <Flex mt="2">.
WalkthroughThe changes primarily involve updates to several components and hooks within the codebase, focusing on the management of secrets and workflow processes. Key modifications include the renaming of a method in the secrets store, the introduction of new hooks and properties in the workflow editor, and enhancements to the workflow saving mechanism. Additionally, new logic has been added to handle deleted secrets more effectively, improving state management across the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ActionEditPanel
participant WorkflowEditor
participant SecretsStore
User->>ActionEditPanel: Select secret or save workflow
ActionEditPanel->>SecretsStore: Call removeDeletedSecretByPlaceholder
SecretsStore-->>ActionEditPanel: Update state
ActionEditPanel->>User: Show updated selection or message
User->>WorkflowEditor: View workflow
WorkflowEditor->>SecretsStore: Retrieve secrets
SecretsStore-->>WorkflowEditor: Provide secrets
WorkflowEditor->>User: Display workflow with secrets
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
web/src/hooks/use-get-workflow-api.ts (1)
10-11: Add documentation for the DISABLE_CACHE constant.While the constant effectively addresses the magic number concern, it would be helpful to add a comment explaining why cache disabling is necessary in this context.
+// Set to 0 to disable caching, ensuring we always fetch fresh workflow data +// This is important for maintaining consistency with the workflow editor state const DISABLE_CACHE = 0;web/src/components/secrets/secrets.tsx (2)
26-31: Consider unconditional cleanup.The cleanup function is currently only registered when
encryptedSecretsexists. Consider moving it outside the conditional to ensure cleanup always occurs:useEffect(() => { if (encryptedSecrets) { setSecrets(encryptedSecrets); - return () => clearSecretsStore(); } + return () => clearSecretsStore(); }, [encryptedSecrets, setSecrets, clearSecretsStore]);
Line range hint
33-35: Consider adding a loading indicator.Currently, the component returns null while loading. Consider adding a loading spinner or skeleton to improve user experience.
if (isListingSecretsLoading) { - return null; + return <LoadingSpinner />; // or <Skeleton /> }web/src/components/workflow-graph/node-base.tsx (1)
66-97: Consider improving accessibility and simplifying the warning indicator structure.While the implementation works, there are a few areas that could be enhanced:
- Add ARIA attributes for better accessibility
- Consider using CSS variables for positioning instead of hardcoded pixel values
- Simplify the nested Flex structure
Here's a suggested improvement:
<NodeToolbar isVisible={hasDeletedSecret(nodeId)} position={Position.Top} offset={8} align="end" > - <Tooltip content="Missing secret"> + <Tooltip content="Missing secret" role="alert"> <Flex + className="transform translate-x-2.5 translate-y-6" style={{ - transform: "translate(10px, 24px)", + "--warning-size": "24px", + "--icon-size": "14px" }} > - <Flex - inset="0" - className="absolute bg-[var(--red-9)] rounded-full" - /> - <Flex - className="relative rounded-full" - width="24px" - height="24px" - justify="center" - align="center" - > + <Flex + className="relative bg-[var(--red-9)] rounded-full flex items-center justify-center" + width="var(--warning-size)" + height="var(--warning-size)" + aria-label="Secret missing warning" + > <ExclamationTriangleIcon className="text-white" - width="14px" - height="14px" + width="var(--icon-size)" + height="var(--icon-size)" /> </Flex> </Flex> </Tooltip> </NodeToolbar>The changes:
- Add ARIA attributes for better screen reader support
- Use CSS variables for consistent sizing
- Use Tailwind classes for transforms
- Simplify the nested Flex structure
- Add semantic role="alert" for accessibility
web/src/providers/save-workflow.tsx (1)
Line range hint
71-89: Consider extracting the argument filtering logic.The argument cleanup logic is complex and could benefit from being extracted into a separate function for better maintainability.
Consider refactoring like this:
+const filterValidActionArgs = ( + args: Record<string, string>, + validArgs: Set<string> +): Record<string, string> => { + return Object.keys(args) + .filter((argName) => validArgs.has(argName)) + .reduce( + (acc, argName) => { + acc[argName] = args[argName]; + return acc; + }, + {} as Record<string, string> + ); +}; if (node.type === EditorWorkflowNodeType.ACTION) { const argsFilter = new Set( actionsIndex[node.actionType].arguments.map( (arg) => arg.argName, ), ); - const args = Object.keys(node.args) - .filter((argName) => argsFilter.has(argName)) - .reduce( - (acc, argName) => { - acc[argName] = node.args[argName]; - return acc; - }, - {} as Record<string, string>, - ); + const args = filterValidActionArgs(node.args, argsFilter); return { ...node, args, }; }web/src/components/workflow-editor/workflow-editor.tsx (1)
114-142: Consider extracting the deleted secrets detection logic.While the implementation is correct and efficiently uses Set/Map for lookups, the effect's complexity could benefit from extraction into a separate utility function for better readability and testability.
Consider refactoring like this:
const detectDeletedSecrets = ( nodes: WorkflowNode[], secretIds: Set<string> ): Map<string, Set<string>> => { return nodes .filter((node) => node.type === "action") .reduce((acc, node) => { const secretsMapping = node.secretsMapping; const secretPlaceholders = Object.keys(secretsMapping); secretPlaceholders.forEach((placeholder) => { if (!secretIds.has(secretsMapping[placeholder])) { if (!acc.has(node.id)) { acc.set(node.id, new Set()); } acc.get(node.id)?.add(placeholder); } }); return acc; }, new Map<string, Set<string>>()); }; // In the effect: useEffect(() => { if (!workflow?.nodes || !encryptedSecrets) { return; } const secretIds = new Set(encryptedSecrets.map((s) => s.secretId)); const deletedSecrets = detectDeletedSecrets(workflow.nodes, secretIds); if (deletedSecrets.size > 0) { setDeletedSecrets(deletedSecrets); errorToast( "Some secrets are missing. Please add them before running the workflow." ); } }, [encryptedSecrets, workflow, setDeletedSecrets]);web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx (3)
70-75: Consider adding error feedback for failed save operations.The save operation correctly handles success, but users might benefit from feedback when the save fails.
const saveWorkflowAndRedirect = async () => { const saveSuccessful = await saveWorkflow(); if (saveSuccessful) { router.push("/settings"); + } else { + // Consider using your notification system here + console.error("Failed to save workflow"); } };
81-84: Extract magic string to constant.The special value "$$$save_and_redirect$$$" should be extracted to a named constant for better maintainability and reuse.
+const SAVE_AND_REDIRECT_ACTION = "$$$save_and_redirect$$$" as const; + const onChangeSecretsMapping = ( secretPlaceholder: string, value: string, ) => { - if (value === "$$$save_and_redirect$$$") { + if (value === SAVE_AND_REDIRECT_ACTION) { saveWorkflowAndRedirect(); return; }
204-218: Enhance accessibility for the warning message.Consider adding appropriate ARIA attributes to improve screen reader experience for the warning message.
<Flex mt="2" direction="row" align="center" + role="alert" + aria-live="polite" > <Text color="red" weight="medium"> The previously selected secret could not be found. </Text> </Flex>web/src/stores/workflow-store.ts (3)
61-61: Fix typo in parameter nameThere's a typo in the parameter name 'secretPlaceholer' (missing 'd'). This should be fixed for consistency.
Apply this fix:
- hasDeletedSecret: (nodeId: string, secretPlaceholer?: string) => boolean; + hasDeletedSecret: (nodeId: string, secretPlaceholder?: string) => boolean;- secretPlaceholer: string, + secretPlaceholder: string,Also applies to: 93-98
399-412: Improve variable naming consistencyThe variable name
secretPlaceholdersWithDeletedSecretsshould match the method name's terminology for better consistency.Apply this change:
- const secretPlaceholdersWithDeletedSecrets = + const deletedSecrets = draft.deletedSecretsForNodes?.get(nodeId); - secretPlaceholdersWithDeletedSecrets?.delete(secretPlaceholder); - if (secretPlaceholdersWithDeletedSecrets?.size === 0) { + deletedSecrets?.delete(secretPlaceholder); + if (deletedSecrets?.size === 0) { draft.deletedSecretsForNodes?.delete(nodeId); }
Line range hint
163-187: Handle deletedSecretsForNodes in setWorkflowThe
setWorkflowmethod should handle thedeletedSecretsForNodesstate to maintain consistency with other workflow properties.Apply this change:
draft.isActive = workflow.isActive; draft.nodes = workflow.nodes; draft.edges = workflow.edges; + draft.deletedSecretsForNodes = undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
web/src/components/secrets/secrets.tsx(1 hunks)web/src/components/workflow-editor/edit-panel/action-edit-panel.tsx(4 hunks)web/src/components/workflow-editor/workflow-editor.tsx(4 hunks)web/src/components/workflow-graph/node-base.tsx(3 hunks)web/src/hooks/use-get-workflow-api.ts(2 hunks)web/src/providers/save-workflow.tsx(4 hunks)web/src/stores/secrets-store.ts(2 hunks)web/src/stores/workflow-store.ts(8 hunks)
🔇 Additional comments (14)
web/src/hooks/use-get-workflow-api.ts (1)
34-34: LGTM! Cache control implementation looks good.
The addition of gcTime: DISABLE_CACHE ensures that the workflow data is always fresh, which is crucial for maintaining consistency with the secret selection dropdown and deleted secret warnings introduced in this PR.
web/src/stores/secrets-store.ts (2)
19-22: Implementation correctly matches the type definition.
The implementation maintains the same functionality while using the new method name.
7-7: Method rename improves clarity but verify all usages.
The rename from clear to clearSecretsStore is more descriptive and better indicates the method's purpose.
Let's verify all usages have been updated:
✅ Verification successful
Method rename from clear to clearSecretsStore has been properly implemented
Based on the verification results:
- The only usage of
clearSecretsStoreis inworkflow-editor.tsxwhere it's correctly using the new name - The
clear()method found inapi-keys.tsxis from a different store (useApiKeysStore) and is unrelated - No other instances of the old method name were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usages of the old method name
# Expect: No occurrences of `.clear()` calls on the secrets store
# Test 1: Search for direct clear method calls
rg -A 2 "useSecretsStore.*clear\("
# Test 2: Search for destructured clear method usage
rg -A 2 "const.*\{.*clear.*\}.*=.*useSecretsStore"
Length of output: 337
Script:
#!/bin/bash
# Let's verify if there are any other usages of the old 'clear' method name
# by searching for it in combination with secrets-related terms
# Search for any remaining references to 'clear' method in combination with 'secrets'
rg -A 2 "clear.*secrets|secrets.*clear" --type ts --type tsx
# Search for any destructured clear method from useSecretsStore with different patterns
rg -A 2 "useSecretsStore.*\{[^\}]*clear[^\}]*\}" --type ts --type tsx
# Search for any direct references to clear in test files that might be related to secrets
rg -A 2 "clear.*\(" "**/*test*" "**/*spec*"
Length of output: 336
Script:
#!/bin/bash
# Let's try a different approach without file type specifications
# and use proper glob patterns for test files
# Search for any remaining references to 'clear' method in combination with 'secrets'
rg -A 2 "clear.*secrets|secrets.*clear"
# Search for any destructured clear method from useSecretsStore
rg -A 2 "useSecretsStore.*\{[^\}]*clear[^\}]*\}"
# Search for test files using fd and then search within them
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx | xargs rg -A 2 "clear.*\("
Length of output: 171
Script:
#!/bin/bash
# Let's do one final check using ast-grep to find any potential usages
# of the old 'clear' method name in the codebase
# Search for any method calls named 'clear' in TypeScript/JavaScript files
ast-grep --pattern 'clear($$$)'
# Search for any property access with 'clear'
ast-grep --pattern '$_.clear'
# Search for any destructuring patterns with 'clear'
ast-grep --pattern '{ $$$, clear, $$$ }'
Length of output: 177
Script:
#!/bin/bash
# Let's check the context of the clear() usage in api-keys.tsx
# to ensure it's not related to the secrets store
# Get the content of the api-keys file with context
rg -B 10 -A 5 "clear\(\)" web/src/components/api-keys/api-keys.tsx
# Also check for any imports or store usage in this file
rg "useSecretsStore|from.*stores/secrets-store" web/src/components/api-keys/api-keys.tsx
Length of output: 554
web/src/components/secrets/secrets.tsx (2)
18-24: LGTM! Clear and descriptive method naming.
The rename from clear to clearSecretsStore improves code clarity by making the method's purpose more explicit.
Line range hint 1-85: Verify integration with workflow warnings.
Since this component is part of the secrets management system that will show warnings for deleted secrets in workflows, we should verify the integration points.
✅ Verification successful
Integration with workflow warnings is properly implemented
The verification shows that the secrets component is well-integrated with the workflow system:
- The workflow editor (
workflow-editor.tsx) checks for missing secrets and validates them before running workflows - There's proper secret mapping and validation in the action edit panel
- The system tracks deleted secrets and shows appropriate UI feedback when secrets are missing
The integration points include:
- Secret validation before workflow execution
- Secret mapping in action configurations
- Proper usage of the
useSecretsStorehook in workflow components - UI components to select and manage secrets within workflow actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for integration between secrets and workflow components
# Look for workflow components that might use these secrets
echo "Checking for workflow components that consume secrets..."
rg -l "useSecretsStore|getSecrets" "web/src/components/workflow"
# Look for warning-related code in workflow components
echo "Checking for warning implementations..."
rg "warning|deleted.*secret" "web/src/components/workflow"
Length of output: 471
Script:
#!/bin/bash
# Let's first find the correct workflow-related directories
echo "Finding workflow-related directories..."
fd -t d "workflow"
# Then search for secret-related code in those directories
echo "Searching for secret-related code..."
rg -l "secret|credential" --type tsx --type ts
# Look for any warning or validation components
echo "Searching for warning/validation components..."
fd -e tsx -e ts "warning|validation|validator"
Length of output: 931
Script:
#!/bin/bash
# Let's check the workflow validation file for secret-related validations
echo "Checking workflow validation..."
cat web/src/lib/workflow-validation.ts
# Let's also check workflow editor components for secret usage
echo "Checking workflow editor components..."
rg -g "*.{ts,tsx}" "secret|credential" web/src/components/workflow-editor/
# Check for any secret-related warnings in the workflow components
echo "Checking for secret warnings..."
rg -g "*.{ts,tsx}" "warning.*secret|deleted.*secret|missing.*secret" web/src/components/workflow-editor/
Length of output: 4343
web/src/components/workflow-graph/node-base.tsx (1)
6-6: LGTM: Clean implementation of secret validation hook.
The addition of ExclamationTriangleIcon and hasDeletedSecret hook is well-structured and aligns with the PR's objective to show warnings for deleted secrets.
Also applies to: 23-24
web/src/providers/save-workflow.tsx (2)
Line range hint 105-118: LGTM! Error handling looks solid.
The error handling is comprehensive and provides good user feedback:
- Handles API errors with specific messages
- Handles validation errors
- Provides fallback error message
- Consistently returns false for all error cases
23-23: Verify the impact of the saveWorkflow return type change.
The change from Promise<void> to Promise<boolean> improves error handling but might affect existing consumers of this context.
Also applies to: 28-28
✅ Verification successful
Return type change is properly handled in all consumers
The search results show that all consumers of saveWorkflow are handling the boolean return value appropriately:
action-edit-panel.tsxexplicitly checks the return value withif (saveSuccessful)new-workflow-modal.tsxdoesn't need the return value for its use caseuse-save-workflow-api.tsis just forwarding the call
The type change is safe and improves error handling without breaking existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of saveWorkflow to ensure all consumers handle the new boolean return value
rg -A 3 "saveWorkflow\(" --type tsx
Length of output: 65
Script:
#!/bin/bash
# Search for usages of saveWorkflow with proper file type
rg -t ts -t tsx -A 3 "saveWorkflow\("
Length of output: 67
Script:
#!/bin/bash
# Search for usages of saveWorkflow using ripgrep without file type filters
rg -A 3 "saveWorkflow\(" web/src/
Length of output: 860
web/src/components/workflow-editor/workflow-editor.tsx (3)
4-4: LGTM: Import statements are well-organized.
The new imports for secrets management functionality are properly structured and follow the project's conventions.
Also applies to: 8-8
41-42: LGTM: Store hooks are properly destructured.
The implementation follows the suggested naming convention for clearSecretsStore, maintaining consistency with the workflow store naming pattern.
Also applies to: 45-45
63-70: LGTM: Secrets management effect is well-implemented.
The effect properly manages the lifecycle of secrets data:
- Sets secrets when data is available
- Includes proper cleanup on unmount
- Has correct dependency array
web/src/stores/workflow-store.ts (3)
13-13: LGTM: Proper initialization of immer for Map/Set support
The addition of enableMapSet from immer and its initialization is correct and necessary for immutable updates to Map and Set data structures.
Also applies to: 51-51
387-392: LGTM: Clean implementation of hasDeletedSecret
The implementation correctly handles both node-level and placeholder-level secret checks with proper null safety.
393-398: LGTM: Proper implementation of setDeletedSecrets
The implementation follows the store's immutable update pattern using immer's produce.
web/src/providers/save-workflow.tsx
Outdated
| @@ -42,7 +42,7 @@ export function SaveWorkflowProvider({ | |||
|
|
|||
| const handleSaveWorkflow = async () => { | |||
| if (isPending) { | |||
| return; | |||
| return false; | |||
There was a problem hiding this comment.
Fix workflow name validation control flow.
The workflow name validation check doesn't return early, allowing invalid workflows to proceed with the save operation. This could lead to inconsistent state.
Apply this fix:
if (!isValidWorkflowName(workflow.workflowName)) {
errorToast(WORKFLOW_NAME_VALIDATION_ERROR_MESSAGE);
+ return false;
}Also applies to: 59-59, 60-60, 61-61, 62-62
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/src/providers/save-workflow.tsx (2)
106-106: Consider enhancing error logging for unknown errors.While the success/failure return values are good, consider adding error logging for unknown errors to help with debugging production issues.
} else { + console.error('Unexpected error while saving workflow:', error); errorToast("Failed to save workflow. Please try again."); }Also applies to: 120-120
Line range hint
34-124: Consider extracting workflow validation logic for better testability.The validation logic could be extracted into a separate service/hook to make it easier to test and maintain. This would also allow for better mocking in tests.
Example structure:
// useWorkflowValidation.ts export function useWorkflowValidation() { return { validateWorkflow: (workflow: Workflow, actionsIndex: ActionsIndex) => { if (Object.keys(actionsIndex).length === 0) { throw new WorkflowValidationError( "Editor actions must be loaded to save the workflow." ); } // ... other validation rules } }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/providers/save-workflow.tsx(5 hunks)
🔇 Additional comments (2)
web/src/providers/save-workflow.tsx (2)
23-23: LGTM! Good improvement to the return type.
The change from void to boolean return type improves error handling by allowing consumers to react to save failures, which is particularly useful for the secrets dropdown feature.
Also applies to: 28-28
45-46: LGTM! Good prevention of concurrent saves.
The early return when a save is already pending prevents race conditions and potential data inconsistencies.
| align="center" | ||
| > | ||
| <ExclamationTriangleIcon | ||
| className="text-white" |
There was a problem hiding this comment.
Radix UI icons have a color prop. Why not use this?
| className="absolute bg-[var(--red-9)] rounded-full" | ||
| /> | ||
| <Flex | ||
| className="relative rounded-full" |
There was a problem hiding this comment.
position exists as a prop in <Flex>, so please move absolute and relative out of className
web/src/providers/save-workflow.tsx
Outdated
| @@ -42,7 +42,7 @@ export function SaveWorkflowProvider({ | |||
|
|
|||
| const handleSaveWorkflow = async () => { | |||
| if (isPending) { | |||
| return; | |||
| return false; | |||
There was a problem hiding this comment.
I think it is okay to remove the if-condition. It is probably more dangerous to return false in this case because if we would do error handling based on the result, it would trigger an error although there is none.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
web/src/components/workflow-graph/node-base.tsx (1)
66-99: Consider simplifying the warning indicator structureWhile the implementation works well, the nested Flex components could be simplified for better maintainability.
Consider this more streamlined structure:
<NodeToolbar isVisible={hasDeletedSecret(nodeId)} position={Position.Top} offset={8} align="end" > <Tooltip content="Missing Secret"> - <Flex - style={{ - transform: "translate(10px, 24px)", - }} - > - <Flex - inset="0" - position="absolute" - className="bg-[var(--red-9)] rounded-full" - /> - <Flex - position="relative" - className="rounded-full" - width="24px" - height="24px" - justify="center" - align="center" - > - <ExclamationTriangleIcon - color="white" - width="14px" - height="14px" - /> - </Flex> - </Flex> + <Flex + className="rounded-full bg-[var(--red-9)]" + style={{ + transform: "translate(10px, 24px)", + }} + width="24px" + height="24px" + justify="center" + align="center" + > + <ExclamationTriangleIcon + color="white" + width="14px" + height="14px" + /> + </Flex> </Tooltip> </NodeToolbar>web/src/providers/save-workflow.tsx (2)
103-103: Consider adding type safety for webhook response.While the success path is clear, consider adding type safety for the webhook response to prevent potential runtime issues.
-const { webhookId, webhookSecret } = await saveWorkflow.mutateAsync( +interface WebhookResponse { + webhookId?: string; + webhookSecret?: string; +} +const { webhookId, webhookSecret } = await saveWorkflow.mutateAsync<WebhookResponse>(
Line range hint
105-117: Improve type safety in error handling.The error handling is comprehensive but relies on unsafe type assertions. Consider defining proper types for the API error response.
+interface WorkflowApiError { + detail: string; +} + if ( error instanceof AxiosError && - (error.response?.data as any).detail.startsWith( + (error.response?.data as WorkflowApiError).detail.startsWith( "A workflow with the name", ) ) { - errorToast((error.response?.data as any).detail); + errorToast((error.response?.data as WorkflowApiError).detail);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/src/components/workflow-graph/node-base.tsx(3 hunks)web/src/providers/save-workflow.tsx(4 hunks)
🔇 Additional comments (4)
web/src/components/workflow-graph/node-base.tsx (2)
6-6: LGTM: Clean integration of new functionality
The addition of ExclamationTriangleIcon and hasDeletedSecret hook is well-integrated and aligns with the PR's objective of showing warnings for deleted secrets.
Also applies to: 23-24
74-76: Verify warning indicator positioning
The use of hardcoded transform values might cause positioning issues in different scenarios.
Let's verify the positioning behavior:
✅ Verification successful
Transform values are intentional for warning indicator positioning
The transform values are used specifically for positioning the warning indicator (ExclamationTriangleIcon) within a NodeToolbar component from reactflow. This is a deliberate positioning choice that:
- Only affects the warning indicator tooltip
- Is contained within a NodeToolbar component that already handles responsive positioning
- Works in conjunction with other layout properties (offset={8}, position={Position.Top}, align="end")
The implementation shows this is a specific UI enhancement rather than a potential positioning issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for responsive design considerations and potential positioning issues
# Test 1: Look for viewport-related styling
rg -l "viewport|media|screen" --type tsx
# Test 2: Check for other transform usages that might conflict
rg "transform:" --type tsx
# Test 3: Look for related test files
fd "node-base.test" --extension tsx --extension ts
Length of output: 176
Script:
#!/bin/bash
# Let's try without file type restrictions since tsx wasn't recognized
# Check for viewport-related styling and responsive design patterns
rg -l "viewport|media|screen" web/src/
# Look for transform usage patterns in the component and related files
rg "transform:" web/src/components/workflow-graph/
# Check the component's implementation for context
cat web/src/components/workflow-graph/node-base.tsx
# Look for test files without extension restrictions
find web/src -name "*node-base.test*"
Length of output: 3814
web/src/providers/save-workflow.tsx (2)
23-23: LGTM! Good improvement to the API contract.
The change from Promise<void> to Promise<boolean> provides better control flow by allowing callers to handle success/failure cases explicitly.
Also applies to: 28-28
59-61: LGTM! Improved error handling approach.
Using WorkflowValidationError instead of direct error toasts is a better practice as it:
- Separates validation logic from UI feedback
- Allows for consistent error handling in the catch block
- Makes the code more testable
| const deletedSecrets = workflow?.nodes | ||
| .filter((node) => node.type === "action") | ||
| .reduce((acc, node) => { | ||
| const secretsMapping = node.secretsMapping; | ||
| const secretPlaceholders = Object.keys(secretsMapping); | ||
| secretPlaceholders.forEach((secretPlaceholder) => { | ||
| if (!secretIds.has(secretsMapping[secretPlaceholder])) { | ||
| if (!acc.has(node.id)) { | ||
| acc.set(node.id, new Set()); | ||
| } | ||
| acc.get(node.id)?.add(secretPlaceholder); | ||
| } | ||
| }); | ||
| return acc; | ||
| }, new Map<string, Set<string>>()); |
There was a problem hiding this comment.
We can write this in a cleaner way:
| const deletedSecrets = workflow?.nodes | |
| .filter((node) => node.type === "action") | |
| .reduce((acc, node) => { | |
| const secretsMapping = node.secretsMapping; | |
| const secretPlaceholders = Object.keys(secretsMapping); | |
| secretPlaceholders.forEach((secretPlaceholder) => { | |
| if (!secretIds.has(secretsMapping[secretPlaceholder])) { | |
| if (!acc.has(node.id)) { | |
| acc.set(node.id, new Set()); | |
| } | |
| acc.get(node.id)?.add(secretPlaceholder); | |
| } | |
| }); | |
| return acc; | |
| }, new Map<string, Set<string>>()); | |
| const deletedSecrets = new Map(workflow?.nodes | |
| .filter((node) => node.type === "action") | |
| .map((node) => { | |
| const secretsMapping = node.secretsMapping; | |
| const deletedSecrets = new Set(Object.keys(secretsMapping).filter((secretPlaceholder) => !secretIds.has(secretsMapping[secretPlaceholder]))); | |
| return [node.id, deletedSecrets]; | |
| }) | |
| .filter((nodeIdAndDeletedSecrets) => nodeIdAndDeletedSecrets[1].size > 0)); |
There was a problem hiding this comment.
Maybe also check what happens if workflow is undefined, i.e., new Map(undefined).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/src/components/workflow-editor/workflow-editor.tsx(4 hunks)
🔇 Additional comments (2)
web/src/components/workflow-editor/workflow-editor.tsx (2)
4-4: LGTM: Clean integration of secrets management.
The new imports and hook usage are well-organized and follow React best practices.
Also applies to: 8-8, 41-42, 45-45
63-70: LGTM: Well-implemented secrets management effect.
The effect properly manages the lifecycle of secrets with appropriate cleanup.
| useEffect(() => { | ||
| if (!workflow || !encryptedSecrets) { | ||
| return; | ||
| } | ||
| const secretIds = new Set(encryptedSecrets.map((s) => s.secretId)); | ||
| const deletedSecrets = new Map( | ||
| workflow?.nodes | ||
| .filter((node) => node.type === "action") | ||
| .map((node) => { | ||
| const secretsMapping = node.secretsMapping; | ||
| const deletedSecrets = new Set( | ||
| Object.keys(secretsMapping).filter( | ||
| (secretPlaceholder) => | ||
| !secretIds.has( | ||
| secretsMapping[secretPlaceholder], | ||
| ), | ||
| ), | ||
| ); | ||
| return [node.id, deletedSecrets] as [string, Set<string>]; | ||
| }) | ||
| .filter( | ||
| (nodeIdAndDeletedSecrets) => | ||
| nodeIdAndDeletedSecrets[1].size > 0, | ||
| ), | ||
| ); | ||
| if (deletedSecrets) { | ||
| setDeletedSecrets(deletedSecrets); | ||
| } | ||
| if (deletedSecrets && deletedSecrets.size > 0) { | ||
| errorToast( | ||
| "Some secrets are missing. Please add them before running the workflow.", | ||
| ); | ||
| } | ||
| }, [encryptedSecrets, workflow, setDeletedSecrets]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve deleted secrets detection implementation.
The logic for detecting deleted secrets can be simplified and improved:
- Remove the redundant
deletedSecretscheck at line 139. - Combine the conditions for setting deleted secrets and showing the toast.
- Enhance the error message to be more specific.
- Use proper typing to avoid type assertion.
Here's the suggested implementation:
useEffect(() => {
if (!workflow || !encryptedSecrets) {
return;
}
const secretIds = new Set(encryptedSecrets.map((s) => s.secretId));
const deletedSecrets = new Map(
workflow?.nodes
.filter((node) => node.type === "action")
.map((node) => {
const secretsMapping = node.secretsMapping;
const deletedSecrets = new Set(
Object.keys(secretsMapping).filter(
(secretPlaceholder) =>
!secretIds.has(
secretsMapping[secretPlaceholder],
),
),
);
- return [node.id, deletedSecrets] as [string, Set<string>];
+ return [node.id, deletedSecrets];
})
.filter(
(nodeIdAndDeletedSecrets) =>
nodeIdAndDeletedSecrets[1].size > 0,
),
);
- if (deletedSecrets) {
- setDeletedSecrets(deletedSecrets);
- }
- if (deletedSecrets && deletedSecrets.size > 0) {
+ setDeletedSecrets(deletedSecrets);
+ if (deletedSecrets.size > 0) {
+ const nodeCount = deletedSecrets.size;
errorToast(
- "Some secrets are missing. Please add them before running the workflow.",
+ `Missing secrets detected in ${nodeCount} ${
+ nodeCount === 1 ? 'node' : 'nodes'
+ }. Please add them before running the workflow.`,
);
}
}, [encryptedSecrets, workflow, setDeletedSecrets]);📝 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.
| useEffect(() => { | |
| if (!workflow || !encryptedSecrets) { | |
| return; | |
| } | |
| const secretIds = new Set(encryptedSecrets.map((s) => s.secretId)); | |
| const deletedSecrets = new Map( | |
| workflow?.nodes | |
| .filter((node) => node.type === "action") | |
| .map((node) => { | |
| const secretsMapping = node.secretsMapping; | |
| const deletedSecrets = new Set( | |
| Object.keys(secretsMapping).filter( | |
| (secretPlaceholder) => | |
| !secretIds.has( | |
| secretsMapping[secretPlaceholder], | |
| ), | |
| ), | |
| ); | |
| return [node.id, deletedSecrets] as [string, Set<string>]; | |
| }) | |
| .filter( | |
| (nodeIdAndDeletedSecrets) => | |
| nodeIdAndDeletedSecrets[1].size > 0, | |
| ), | |
| ); | |
| if (deletedSecrets) { | |
| setDeletedSecrets(deletedSecrets); | |
| } | |
| if (deletedSecrets && deletedSecrets.size > 0) { | |
| errorToast( | |
| "Some secrets are missing. Please add them before running the workflow.", | |
| ); | |
| } | |
| }, [encryptedSecrets, workflow, setDeletedSecrets]); | |
| useEffect(() => { | |
| if (!workflow || !encryptedSecrets) { | |
| return; | |
| } | |
| const secretIds = new Set(encryptedSecrets.map((s) => s.secretId)); | |
| const deletedSecrets = new Map( | |
| workflow?.nodes | |
| .filter((node) => node.type === "action") | |
| .map((node) => { | |
| const secretsMapping = node.secretsMapping; | |
| const deletedSecrets = new Set( | |
| Object.keys(secretsMapping).filter( | |
| (secretPlaceholder) => | |
| !secretIds.has( | |
| secretsMapping[secretPlaceholder], | |
| ), | |
| ), | |
| ); | |
| return [node.id, deletedSecrets]; | |
| }) | |
| .filter( | |
| (nodeIdAndDeletedSecrets) => | |
| nodeIdAndDeletedSecrets[1].size > 0, | |
| ), | |
| ); | |
| setDeletedSecrets(deletedSecrets); | |
| if (deletedSecrets.size > 0) { | |
| const nodeCount = deletedSecrets.size; | |
| errorToast( | |
| `Missing secrets detected in ${nodeCount} ${ | |
| nodeCount === 1 ? 'node' : 'nodes' | |
| }. Please add them before running the workflow.`, | |
| ); | |
| } | |
| }, [encryptedSecrets, workflow, setDeletedSecrets]); |
danielgrittner
deleted the
ADM-369/Secrets-list-as-dropdown-from-existing-secrets
branch
This PR introduces a new SaveWorkflowProvder which is used in the save-workflow-button for instacen.
Additionally, the user can now select a secret for an action node via a dropdown showing the users's saved secrets.
Should a previously selected secret be deleted, it will show as a warning for the particual node and in the respective edit-panel.
Summary by CodeRabbit
Release Notes
New Features
ActionEditPanelwith a dropdown for selecting secrets and an option to save and redirect.NodeBasefor nodes with missing secrets.Secretscomponent for better secret management and cleanup.Bug Fixes
Documentation
Chores