WEB: Previous account cluster list are showing while account switching #280
Conversation
- fix cluster listing while creating environments and integrated services after switching account
Reviewer's Guide by SourceryThis pull request implements changes related to cloning environments and updating various components in the Kloudlite console application. The changes include adding a new mutation for cloning environments, updating the environment handling component, and modifying the managed service layout. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nxtCoder19 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🔴 Security: 2 blocking issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| } | ||
| } | ||
| `, | ||
| { |
There was a problem hiding this comment.
issue: Inconsistent error handling in transformer functions
The transformer function in cli_listByokClusters directly returns the data, while cli_cloneEnvironment doesn't have a transformer. This inconsistency could lead to unexpected behavior or difficulties in error handling. Consider standardizing the approach across both functions.
| @@ -563,7 +563,7 @@ const ManagedServiceLayout = () => { | |||
|
|
|||
| useEffect(() => { | |||
| getClusters(); | |||
| }, []); | |||
There was a problem hiding this comment.
suggestion (performance): Dependency array in useEffect includes clusterList
The useEffect hook now includes clusterList in its dependency array. This could cause the effect to run more often than intended. Consider if this change is necessary and if it might introduce any performance issues.
| ), | ||
| }, | ||
| })); | ||
| } | ||
| }, [values.selectedTemplate]); | ||
|
|
||
| useEffect(() => { | ||
| if (clusterList.length > 0) { |
There was a problem hiding this comment.
question: More defensive cluster name setting logic
The new version of setting clusterName is more defensive, handling the case where clusterList is empty. Can you explain the reasoning behind this change? Are there specific scenarios or issues this is addressing?
| id: string; | ||
| ipAddr: string; | ||
| markedForDeletion?: boolean; | ||
| privateKey: string; |
There was a problem hiding this comment.
🚨 issue (security): Avoid hardcoding private keys in the code.
Storing private keys directly in the code can lead to security vulnerabilities. Consider using environment variables or a secure vault to manage sensitive information.
| ipAddr: string; | ||
| markedForDeletion?: boolean; | ||
| privateKey: string; | ||
| publicKey: string; |
There was a problem hiding this comment.
🚨 issue (security): Avoid hardcoding public keys in the code.
While public keys are not as sensitive as private keys, it's still a good practice to manage them securely, especially if they are part of a key pair.
WEB: Previous account cluster list are showing while account switching
WEB: Previous account cluster list are showing while account switching
WEB: Previous account cluster list are showing while account switching
Resolves #279
Summary by Sourcery
Add a new feature to clone environments using a GraphQL mutation and enhance the environment and cluster management components and queries for better state handling and CLI support.
New Features:
core_cloneEnvironmentto clone environments with specified parameters such as cluster name, source and destination environment names, display name, and routing mode.Enhancements:
ManagedServiceLayoutandHandleEnvironmentcomponents to improve state management and effect dependencies, ensuring that cluster lists are updated correctly.cliQueriesto include new queries and mutations for managing environments and clusters, enhancing the CLI capabilities.