Impl/cluster status #107
Impl/cluster status #107
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: The pull request introduces enhancements to the cluster status handling and display within a project creation flow. It includes changes to filter out clusters in the 'deleting' state from the selection options and updates the UI to reflect different cluster statuses ('ready', 'syncing', 'notready') more accurately. Additionally, it involves minor refactoring and updates across various components and queries to support these enhancements.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
- Big diff: the diff is too large to approve with confidence.
General suggestions:
- Consider abstracting complex logic into separate functions or utilizing useMemo for operations that don't need to be recalculated on every render. This can improve code readability and performance.
- Ensure that all components consuming the updated
IStatustype properly handle the new 'ready' status. This might require adjustments in rendering logic, styling, or business logic. - Given the changes in GraphQL queries and mutations, verify that corresponding tests are updated or added to cover these changes, ensuring the reliability of API interactions.
- The introduction of new or updated logging functionality suggests a need for tests verifying logging behavior, especially in critical execution paths or error scenarios.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| .filter( | ||
| (clster) => parseStatus({ item: clster }) !== 'deleting' | ||
| ) | ||
| .map((clster) => ({ | ||
| label: clster.displayName, | ||
| value: parseName(clster), | ||
| cluster: clster, | ||
| render: () => ( | ||
| <div> | ||
| {parseStatus({ item: clster }) === 'ready' ? ( | ||
| <div className="flex flex-col"> | ||
| <div>{clster.displayName}</div> | ||
| <div className="bodySm text-text-soft"> | ||
| {parseName(clster)} | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| // parseStatus({ item: clster }) === 'syncing' || | ||
| // parseStatus({ item: clster }) === 'notready' ? | ||
| <div className="flex text-text-disabled"> | ||
| <div className="flex-grow"> | ||
| <div className="flex flex-col"> | ||
| <div>{clster.displayName}</div> | ||
| <div className="bodySm"> | ||
| {parseName(clster)} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div className="flex flex-grow-0 items-center"> | ||
| {statusRender( | ||
| clster as ExtractNodeType<IClusters> | ||
| ).render()} | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ), | ||
| })), | ||
| ), | ||
| })), |
There was a problem hiding this comment.
suggestion (llm): The filtering and mapping logic for clusters based on their status introduces a significant amount of complexity within the JSX. Consider abstracting this logic into a separate function or utilizing useMemo to avoid recalculating this on every render, especially since this operation depends on the clusters' data which might not change on every render. This could improve readability and performance.
| <div className="bodySm text-text-soft"> | ||
| {parseName(clster)} | ||
| ...clusters | ||
| .filter( |
There was a problem hiding this comment.
suggestion (llm): It seems like the filtering logic for clusters based on their status ('deleting') has been introduced. I recommend adding unit tests to verify that clusters with a status of 'deleting' are indeed filtered out and not displayed as options. This is crucial to ensure the UI behaves as expected and only presents valid clusters to the user.
| <div className="bodySm text-text-soft"> | ||
| {parseName(clster)} | ||
| ...clusters | ||
| .filter( |
There was a problem hiding this comment.
suggestion (llm): Considering the addition of conditional rendering based on the cluster status ('ready', 'syncing', 'notready'), it would be beneficial to include integration or end-to-end tests. These tests should verify that the UI correctly displays the cluster information and status indicators according to the cluster's current state. This ensures that users receive accurate feedback on the cluster status.
| @@ -163,7 +163,7 @@ const SyncStatus = ({ item }: { item: IStatusMeta }) => { | |||
| return null; | |||
| }; | |||
|
|
|||
| export type IStatus = 'deleting' | 'notready' | 'syncing' | 'none'; | |||
| export type IStatus = 'deleting' | 'notready' | 'syncing' | 'ready'; | |||
There was a problem hiding this comment.
suggestion (llm): The modification to the IStatus type to include 'ready' is a significant change. It's important to ensure that all components consuming IStatus properly handle the new 'ready' status. This might include rendering logic, styling, or even business logic changes. If not already covered, adding tests to verify these components react appropriately to the 'ready' status would mitigate potential issues.
| @@ -32,7 +32,7 @@ export const clusterQueries = (executor: IExecutor) => ({ | |||
| { | |||
| transformer: (data: ConsoleCreateClusterMutation) => | |||
| data.infra_createCluster, | |||
| vars(_: ConsoleCreateClusterMutationVariables) { }, | |||
| vars(_: ConsoleCreateClusterMutationVariables) {}, | |||
There was a problem hiding this comment.
suggestion (llm): Given the changes in GraphQL queries and mutations, it's crucial to ensure that the corresponding tests are updated or added to cover these changes. This includes verifying that the queries and mutations behave as expected with the new or modified variables, and that error handling is properly implemented. This is important for maintaining the reliability of the API interactions.
| @@ -32,13 +32,13 @@ import AnimateHide from '~/components/atoms/animate-hide'; | |||
| import { ISetState } from '~/console/page-components/app-states'; | |||
| import { Button } from '~/components/atoms/button'; | |||
| import { dayjs } from '~/components/molecule/dayjs'; | |||
| import LogComp from '~/root/lib/client/components/logger'; | |||
There was a problem hiding this comment.
suggestion (llm): The introduction of a new logging component (LogComp) in the nodepool resources file suggests changes in logging behavior or additional logging functionality. It would be beneficial to have tests that verify the logging behavior, especially in error scenarios or critical execution paths. This ensures that the system's operational aspects are observable and debuggable.
| if (parseStatus({ item: v.cluster }) === 'ready') | ||
| handleChange('clusterName')(dummyEvent(v.value)); |
There was a problem hiding this comment.
suggestion (rule): Sourcery has identified the following issue:
- Use block braces for ifs, whiles, etc. (
use-braces)
| if (parseStatus({ item: v.cluster }) === 'ready') | |
| handleChange('clusterName')(dummyEvent(v.value)); | |
| if (parseStatus({ item: v.cluster }) === 'ready') { | |
| handleChange('clusterName')(dummyEvent(v.value)); | |
| } | |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
No description provided.