feat: implement deployments list table in infra monitoring #6620
Conversation
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
amlannandy
force-pushed
the
feat/infra-monitoring-k8s-nodes
branch
4 times, most recently
from
26a6959 to
b5d57e1
Compare
amlannandy
force-pushed
the
feat/infra-monitoring-k8s-deployments
branch
from
df663a6 to
0dd674b
Compare
amlannandy
changed the base branch from
feat/infra-monitoring-k8s-nodes
to
feat/infra-monitoring-k8s
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 1798c04 in 1 minute and 54 seconds
More details
- Looked at
1222lines of code in9files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. frontend/src/api/infraMonitoring/getK8sDeploymentsList.ts:55
- Draft comment:
Consider checking the response status code to ensure it is 200 before returning a success response. This will help in correctly identifying non-200 responses as errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
- Axios automatically throws for non-200 responses. 2. Those errors are caught and handled by ErrorResponseHandler. 3. If we reach line 60, we definitely have a successful response. 4. Adding an extra status code check would be redundant. 5. The comment shows a misunderstanding of how Axios works.
Maybe there's an edge case where we want to handle certain non-200 status codes differently? Maybe ErrorResponseHandler isn't comprehensive enough?
The ErrorResponseHandler is a centralized error handling utility, and if special handling is needed, it should be implemented there, not in individual API calls. This maintains consistency across the codebase.
The comment should be deleted as it suggests unnecessary code due to misunderstanding Axios behavior, and proper error handling is already in place.
2. frontend/src/container/InfraMonitoringK8s/Deployments/K8sDeploymentsList.tsx:215
- Draft comment:
AddfetchGroupedByRowDataQueryto the dependency array of thisuseEffectto ensure it refetches data when the query changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The useEffect is already correctly set up. fetchGroupedByRowData is a refetch function from useGetK8sDeploymentsList that already has access to the latest query through its closure. The query itself is passed as part of the hook configuration. Adding fetchGroupedByRowDataQuery to the deps array would be redundant since selectedRowData (which is already in deps) drives the query changes.
Could there be edge cases where the query changes but selectedRowData doesn't, requiring a refetch? For example, when minTime/maxTime change?
No - in this case, the useGetK8sDeploymentsList hook will automatically refetch when its query changes, without needing the useEffect to trigger it. The useEffect is only needed to trigger the initial fetch when selectedRowData changes.
The comment should be deleted. The current dependency array is correct, and adding fetchGroupedByRowDataQuery would be redundant since the query updates are already handled properly by the React Query hook.
3. frontend/src/container/InfraMonitoringK8s/Deployments/K8sDeploymentsList.tsx:233
- Draft comment:
Consider resetting the current page to 1 when the sorting changes to ensure the user sees the first page of sorted results. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/container/InfraMonitoringK8s/Deployments/K8sDeploymentsList.tsx:416
- Draft comment:
Consider resetting the current page to 1 when the group by attribute changes to ensure the user sees the first page of grouped results. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/container/InfraMonitoringK8s/K8sHeader.tsx:132
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is also applicable in other parts of the code where inline styles are used. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
6. frontend/src/container/InfraMonitoringK8s/Deployments/utils.tsx:253
-
Draft comment:
This is duplicate functionality. Consider using this new function to replace the inline color logic in the referenced locations to reduce code duplication. -
CPU color threshold logic (HostMetricsDetails.tsx)
-
CPU color threshold logic (utils.tsx)
-
Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_Z0uLpuvoOV1HpaRJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| return options.queryKey; | ||
| } | ||
|
|
||
| return [REACT_QUERY_KEY.GET_HOST_LIST, requestData]; |
There was a problem hiding this comment.
The default query key REACT_QUERY_KEY.GET_HOST_LIST seems incorrect for deployments. Consider using a more specific key for deployments.
| return ( | ||
| <div className="pod-group"> | ||
| {groupByValues.map((value) => ( | ||
| <Tag key={value} color="#1D212D" className="pod-group-tag-item"> |
There was a problem hiding this comment.
Use design tokens or predefined color constants instead of hardcoding color values like #1D212D for consistency in design and theming. This is also applicable in other parts of the code where colors are hardcoded.
amlannandy
force-pushed
the
feat/infra-monitoring-k8s-deployments
branch
from
1798c04 to
0ec6a59
Compare
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 0ec6a59 in 1 minute and 19 seconds
More details
- Looked at
834lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Deployments/utils.tsx:94
- Draft comment:
Avoid using inline styles for column headers. Use external stylesheets, CSS classes, or styled components instead. This is also applicable infrontend/src/container/InfraMonitoringK8s/utils.tsx. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/container/InfraMonitoringK8s/InfraMonitoringK8s.tsx:39
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach. It makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_xUol4lJIMHBLvqpt
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| containerRestarts: number; | ||
| clusterName: string; | ||
| namespaceName: string; | ||
| groupedByMeta?: any; |
There was a problem hiding this comment.
Consider replacing any with a more specific type for groupedByMeta to ensure type safety.
| return ( | ||
| <div className="pod-group"> | ||
| {groupByValues.map((value) => ( | ||
| <Tag key={value} color="#1D212D" className="pod-group-tag-item"> |
There was a problem hiding this comment.
Use design tokens or predefined color constants instead of hardcoding color values for the Tag component. This is also applicable in frontend/src/container/InfraMonitoringK8s/utils.tsx.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6b1f3da in 33 seconds
More details
- Looked at
415lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Deployments/utils.tsx:99
- Draft comment:
Consider changing the type ofclusterNameandnamespaceNametoReact.ReactNodefor consistency, as other fields are typed asReact.ReactNode. - Reason this comment was not posted:
Confidence changes required:50%
The use ofReact.ReactNodefor fields likedeploymentName,availableReplicas, etc., inK8sDeploymentsRowDatais appropriate since these fields are rendered as JSX elements. However, theclusterNameandnamespaceNamefields are still typed asstring, which might be inconsistent if they are also rendered as JSX elements.
2. frontend/src/container/InfraMonitoringK8s/Deployments/utils.tsx:378
- Draft comment:
Consider wrappingclusterNameandnamespaceNamewithValidateColumnValueWrapperto handle potential null or undefined values consistently. - Reason this comment was not posted:
Confidence changes required:50%
TheformatDataForTablefunction usesValidateColumnValueWrapperfor several fields, which is a good practice for handling potential null or undefined values. However, theclusterNameandnamespaceNamefields are not wrapped, which might lead to inconsistencies if they can also be null or undefined.
3. frontend/src/container/InfraMonitoringK8s/Deployments/utils.tsx:305
- Draft comment:
Avoid using inline styles for the progress bar. Instead, use external stylesheets, CSS classes, or styled components. This is also applicable in other parts of the code where inline styles are used. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_JycQmKxarlUvPjwr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Summary
Implement the deployments list table in Infra Monitoring
Related Issues / PR's
N/A
Screenshots
N/A
Affected Areas and Manually Tested Areas
Infra Monitoring section
Important
Implement Kubernetes deployments list feature in Infra Monitoring with new API, component, and utilities.
getK8sDeploymentsListingetK8sDeploymentsList.tsto fetch Kubernetes deployments.K8sDeploymentsListPayload,K8sDeploymentsData, andK8sDeploymentsListResponse.K8sDeploymentsListcomponent inK8sDeploymentsList.tsxfor displaying deployments with pagination, sorting, and filtering.useGetK8sDeploymentsListhook inuseGetK8sDeploymentsList.tsfor data fetching.utils.tsxfor data formatting and table column definitions.InfraMonitoringK8s.tsxto includeK8sDeploymentsListcomponent.constants.tsto include deployment-related quick filters.This description was created by
for 6b1f3da. It will automatically update as commits are pushed.