-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement namespaces list table in infra monitoring #6617
base: feat/infra-monitoring-k8s
Are you sure you want to change the base?
feat: implement namespaces list table in infra monitoring #6617
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
ef641e7
to
2e8350b
Compare
0e1f256
to
728f325
Compare
26a6959
to
b5d57e1
Compare
728f325
to
d5cd9f7
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 98e5fe6 in 1 minute and 44 seconds
More details
- Looked at
947
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/api/infraMonitoring/getK8sNamespacesList.ts:53
- Draft comment:
Useresponse.status
instead of hardcoded200
for statusCode to ensure it reflects the actual response status. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
This is a new API endpoint file. The comment suggests a change that could make the code more dynamic, but there are several issues: 1) This is a success case in the try block, so we know it succeeded 2) Axios successful responses always have status 200 3) The statusCode is part of our own response wrapper, not the HTTP status 4) The suggestion could actually introduce bugs if the backend returns a different 2xx status code.
Maybe using response.status would be more accurate for non-200 success codes like 201 Created. The current approach might hide important status information.
This is a GET endpoint that returns a list - it will always be 200 on success. Other status codes would be caught in the catch block and handled by ErrorResponseHandler.
The comment should be deleted. The hardcoded 200 is correct here as it's in the success path, and using response.status could actually introduce bugs.
2. frontend/src/api/infraMonitoring/getK8sNamespacesList.ts:55
- Draft comment:
Useresponse.statusText
instead of hardcoded 'Success' for the message to ensure it reflects the actual response message. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about code style/implementation choice, not a clear bug. Using response.statusText isn't necessarily better - it would make the response message less predictable and consistent. The hardcoded 'Success' is a deliberate choice to provide a consistent response format. HTTP status texts can vary between servers and implementations.
Maybe using the actual HTTP status text would provide more detailed information about the response. The hardcoded message could hide useful information.
The purpose of this message field seems to be providing a consistent, predictable success indicator rather than detailed status information. The actual response data is already included in the payload.
This comment should be deleted as it suggests a change that isn't clearly better and could make the API response less consistent.
Workflow ID: wflow_rotZg7lxf6EgSuYM
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.
98e5fe6
to
a0c8bf9
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Incremental review on a0c8bf9 in 1 minute and 3 seconds
More details
- Looked at
757
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/InfraMonitoringK8s.tsx:324
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This applies to all similar instances in this file. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_UgbcDEXuPKcSMRQY
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 ( | ||
<div className="pod-group"> | ||
{groupByValues.map((value) => ( | ||
<Tag key={value} color="#1D212D" className="pod-group-tag-item"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use design tokens or predefined color constants instead of hardcoding color values in the Tag
component.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 384b309 in 25 seconds
More details
- Looked at
111
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Namespaces/K8sNamespacesList.tsx:325
- Draft comment:
Ensure that settingshowHeader={false}
is intentional. Hiding table headers can reduce clarity for users. - Reason this comment was not posted:
Confidence changes required:50%
TheshowHeader
property is set tofalse
for theTable
component, which might not be the intended behavior if headers are needed for clarity.
2. frontend/src/container/InfraMonitoringK8s/Namespaces/utils.tsx:28
- Draft comment:
Ensure that the renaming ofcpuUsage
tocpu
andmemoryUsage
tomemory
is consistent across the codebase. This change is also applicable inK8sNamespacesRowData
andcolumnsConfig
. - Reason this comment was not posted:
Confidence changes required:50%
Thecpu
andmemory
fields have been renamed fromcpuUsage
andmemoryUsage
. Ensure consistency across the codebase.
Workflow ID: wflow_wV26lGxAs0EeFQm3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 79e07b7 in 12 seconds
More details
- Looked at
24
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/hooks/infraMonitoring/useGetK8sNamespacesList.ts:37
- Draft comment:
The change fromGET_HOST_LIST
toGET_NAMESPACE_LIST
in the query key is correct and aligns with the new functionality of fetching namespaces. - Reason this comment was not posted:
Confidence changes required:0%
The change fromGET_HOST_LIST
toGET_NAMESPACE_LIST
in the query key is correct and aligns with the new functionality of fetching namespaces.
2. frontend/src/hooks/infraMonitoring/useGetK8sNamespacesList.ts:37
- Draft comment:
Ensure thatREACT_QUERY_KEY.GET_NAMESPACE_LIST
is defined inreactQueryKeys.ts
to maintain consistency and avoid hardcoding. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_OBlofEjwc5hhz7Da
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Summary
Implement the namespaces 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 namespaces list table in Infra Monitoring with new API, UI components, and hooks.
getK8sNamespacesList
function ingetK8sNamespacesList.ts
to fetch namespaces list.K8sNamespacesListPayload
,K8sNamespacesData
, andK8sNamespacesListResponse
.K8sNamespacesList
component inInfraMonitoringK8s.tsx
to display namespaces.InfraMonitoringK8s.tsx
to includeK8sNamespacesList
based on selected category.K8sHeader
component for managing filters and group by options.useGetK8sNamespacesList
hook inuseGetK8sNamespacesList.ts
for fetching data using React Query.utils.tsx
for formatting data and defining table columns.constants.ts
to include namespace-related quick filters.This description was created by for 79e07b7. It will automatically update as commits are pushed.