-
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: added the host list view and filters #6210
Conversation
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> |
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! Reviewed everything up to f8eeec6 in 40 seconds
More details
- Looked at
1063
lines of code in29
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. frontend/src/pages/InfraMonitoringHosts/HostsList.tsx:10
- Draft comment:
Missing semicolon at the end of the import statement forHostsListControls
. - Reason this comment was not posted:
Confidence changes required:10%
The import statement forHostsListControls
is missing a semicolon at the end. This is a minor style issue but should be corrected for consistency.
2. frontend/src/pages/InfraMonitoringHosts/HostsList.tsx:30
- Draft comment:
Remove theconsole.log(options);
statement to maintain clean code. - Reason this comment was not posted:
Confidence changes required:10%
The console.log statement is likely used for debugging purposes and should be removed before merging the code to maintain clean code practices.
3. frontend/src/pages/InfraMonitoringHosts/HostsListControls.tsx:27
- Draft comment:
Remove theconsole.log('currentQuery', updatedCurrentQuery);
statement to maintain clean code. - Reason this comment was not posted:
Confidence changes required:10%
Theconsole.log
statement is likely used for debugging and should be removed before merging to maintain clean code practices.
4. frontend/src/pages/InfraMonitoringHosts/utils.tsx:36
- Draft comment:
ThedataIndex
values ingetHostsListColumns
should match the keys in the data source. UsehostName
,cpu
,memory
, anddisk
instead ofhostname
,cpu_usage
,memory_usage
, anddisk_usage
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment assumes that thedataIndex
values do not match the data source keys, but there is no evidence in the provided code to support this claim. Without seeing the data source or additional context, it's speculative to assume the suggested changes are necessary. The comment does not provide strong evidence that a code change is required.
The data source is not visible in the provided code, so it's possible that the currentdataIndex
values are correct. The comment might be based on an assumption rather than evidence.
Without evidence from the data source, it's safer to assume the currentdataIndex
values are correct. The comment lacks strong evidence to support its claim.
Delete the comment as it lacks strong evidence and is speculative about the need for changes todataIndex
values.
5. frontend/src/pages/InfraMonitoringHosts/HostsList.tsx:60
- Draft comment:
Avoid using inline styles. Use external stylesheets or styled components instead. - Reason this comment was not posted:
Comment was on unchanged code.
6. frontend/src/pages/InfrastructureMonitoring/InfrastructureMonitoring.styles.scss:15
- Draft comment:
Avoid using hardcoded color values. Use design tokens or predefined color constants instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code uses CSS variables (e.g., var(--bg-slate-400)), which are typically used for design tokens or predefined constants. This suggests that the comment may not be applicable, as the code is already following best practices by not hardcoding color values directly.
I might be misunderstanding the nature of the variables used. If these variables are not part of a design system, the comment could be valid. However, CSS variables are generally used for this purpose.
Given the use of CSS variables, it's reasonable to assume they are part of a design system unless there's evidence to the contrary. The comment seems unnecessary.
The comment should be deleted because the code already uses CSS variables, which are likely design tokens or predefined constants.
Workflow ID: wflow_m6k1zhwRpx3LczB7
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> |
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 ae014d1 in 34 seconds
More details
- Looked at
351
lines of code in11
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. frontend/src/api/infraMonitoring/getHostLists.ts:58
- Draft comment:
Ensure that the backend API endpoint '/hosts/list' is correctly configured to handle this request. The change from '/hosts' to '/hosts/list' might break the API call if not updated in the backend. - Reason this comment was not posted:
Comment did not seem useful.
2. frontend/src/constants/localStorage.ts:24
- Draft comment:
The removal ofINFRAMONITORING_HOSTS_LIST_OPTIONS
might affect functionality relying on this key. Ensure it's not used elsewhere in the codebase. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/src/container/HostMetricsLoading/HostMetricsLoading.tsx:14
- Draft comment:
Consider using a constant or configuration for the loading GIF path to avoid issues if the file path changes. - Reason this comment was not posted:
Confidence changes required:50%
TheHostMetricsLoading
component is well-structured, but the hardcoded path for the loading GIF might cause issues if the file path changes. Consider using a constant or configuration for the path.
4. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/index.tsx:101
- Draft comment:
The addition ofisInfraMonitoring
parameter touseAutoComplete
is consistent. Ensure this parameter is used correctly within the hook. - Reason this comment was not posted:
Confidence changes required:30%
TheuseAutoComplete
hook now accepts anisInfraMonitoring
parameter, which is correctly passed in theQueryBuilderSearch
component. This change is consistent across the files whereuseAutoComplete
is used.
5. frontend/src/pages/InfraMonitoringHosts/HostsListControls.tsx:28
- Draft comment:
Removeconsole.log
statements before deploying to production to avoid unnecessary logging. - Reason this comment was not posted:
Confidence changes required:50%
TheHostsList
component usesuseMemo
for memoizing the query and columns, which is a good practice for performance optimization. However, theconsole.log
statement should be removed before production deployment.
6. frontend/src/container/QueryBuilder/filters/QueryBuilderSearch/index.tsx:94
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. 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.
7. frontend/src/pages/InfrastructureMonitoring/InfrastructureMonitoring.styles.scss:8
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values to maintain consistency in design and theming. This is also applicable in other parts of the code where color values are hardcoded. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_VnNQTjsCZN7S0B0j
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> |
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 08512b9 in 26 seconds
More details
- Looked at
249
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/pages/InfraMonitoringHosts/HostsList.tsx:56
- Draft comment:
Consider using a more specific type forpagination
instead ofany
to improve type safety. - Reason this comment was not posted:
Confidence changes required:50%
The use ofany
for thepagination
parameter inhandleTableChange
is not ideal. It's better to use a more specific type to improve type safety and code readability.
2. frontend/src/pages/InfraMonitoringHosts/utils.tsx:76
- Draft comment:
EnsurehostName
is always a valid string. Ifhost.hostName
can be empty, consider handling this case explicitly. - Reason this comment was not posted:
Confidence changes required:50%
ThehostName
field in theHostRowData
interface is marked as a string, but in theformatDataForTable
function, it is being assigned an empty string ifhost.hostName
is falsy. This could lead to inconsistencies ifhostName
is expected to always have a value.
3. frontend/src/pages/InfraMonitoringHosts/utils.tsx:24
- Draft comment:
Ensure that the time conversion to milliseconds is consistent with the rest of the application. If the original times are already in milliseconds, this multiplication is unnecessary. - Reason this comment was not posted:
Confidence changes required:33%
ThegetHostListsQuery
function multiplies the start and end times by 1e3, which converts them to milliseconds. This is correct if the original times are in seconds, but it's important to ensure this is consistent with the rest of the application.
Workflow ID: wflow_xrcUcGSRC66u5LLv
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.
|
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 3d57dde in 29 seconds
More details
- Looked at
91
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/pages/InfraMonitoringHosts/HostsListControls.tsx:39
- Draft comment:
Avoid using inline styles. Use external stylesheets, CSS classes, or styled components instead. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_kvrpqLgZC8OJlOia
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.
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 689440b in 41 seconds
More details
- Looked at
177
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/pages/InfraMonitoringHosts/utils.tsx:116
- Draft comment:
Avoid using inline styles. Consider using external stylesheets or styled components for better maintainability. This is also applicable to line 104. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_vA3uynBkzrtGWieA
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.
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 403fe9d in 47 seconds
More details
- Looked at
186
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. frontend/src/pages/InfraMonitoringHosts/utils.tsx:63
- Draft comment:
ThedataIndex
for 'IOWait' should beioWait
to match theHostRowData
interface. This will ensure consistency and prevent potential bugs. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out a potential inconsistency between thedataIndex
and theHostRowData
interface. However, theformatDataForTable
function mapshost.wait
toioWait
, which means thedataIndex
should indeed bewait
to match the data source. The comment is incorrect because the change aligns thedataIndex
with the data source, not the interface.
I might be missing the importance of aligning thedataIndex
with theHostRowData
interface instead of the data source. However, the functionformatDataForTable
clearly mapshost.wait
toioWait
, indicating thedataIndex
should match the data source.
The mapping informatDataForTable
is a strong indication that thedataIndex
should bewait
, as it reflects the data source structure. The comment is incorrect in suggesting a change toioWait
.
The comment is incorrect because thedataIndex
should match the data source, which iswait
, not theHostRowData
interface. The comment should be deleted.
2. frontend/src/pages/InfraMonitoringHosts/utils.tsx:89
- Draft comment:
Avoid using inline styles in React components. Consider using external stylesheets, CSS classes, or styled components instead. This is also applicable at line 104. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant because it addresses a change made in the diff, specifically the use of inline styles in the formatDataForTable function. The suggestion to use external stylesheets or styled components is a common best practice in React development to improve maintainability and consistency. Therefore, the comment is actionable and clear, aligning with the rules for review comments.
The comment might be considered too general, as it doesn't provide specific guidance on how to refactor the inline styles. However, the suggestion is a well-known best practice, and the comment is relevant to the changes made in the diff.
While the comment could be more specific, it still provides a valid and actionable suggestion that aligns with best practices. The relevance to the changes made in the diff justifies keeping the comment.
Keep the comment as it provides a relevant and actionable suggestion regarding the use of inline styles in the updated code.
3. frontend/src/pages/InfraMonitoringHosts/utils.tsx:104
- Draft comment:
Avoid using inline styles in React components. Consider using external stylesheets, CSS classes, or styled components instead. This is also applicable at line 89. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_4ui1KK6MDreGH8h1
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> |
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 6648e84 in 36 seconds
More details
- Looked at
92
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/pages/InfraMonitoringHosts/HostsList.tsx:101
- Draft comment:
The dependency array foruseCallback
should includesetFilters
to ensure the callback updates whensetFilters
changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is directly related to the change made in the diff, which is the removal ofsetFilters
from the dependency array. The comment suggests that this change might be incorrect because it could lead to issues ifsetFilters
changes. This is a valid concern becauseuseCallback
should include dependencies that are used within the callback to ensure it updates correctly.
The comment might be unnecessary ifsetFilters
is guaranteed not to change, or if the function is intended to be static. However, without more context, it's safer to assume the comment is valid.
Given thatsetFilters
is a state setter function fromuseState
, it is unlikely to change, but the comment is still valid as a precautionary measure.
Keep the comment as it addresses a potential issue with the dependency array ofuseCallback
.
2. frontend/src/pages/InfraMonitoringHosts/HostsListControls.tsx:44
- Draft comment:
The dependency array foruseCallback
should includehandleChangeQueryData
andhandleFiltersChange
to ensure the callback updates when these dependencies change. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. frontend/src/pages/InfraMonitoringHosts/utils.tsx:89
- Draft comment:
Avoid using inline styles. Use CSS classes or styled components instead. This applies to other similar instances in this file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant because it addresses a change made in the diff, where inline styles were replaced with CSS classes. This aligns with best practices for styling in React components. The comment is not redundant or speculative, and it provides a clear suggestion that has been implemented in the code changes.
The comment might be considered unnecessary since the change has already been made, and the code now follows the suggested practice. However, it reinforces the importance of the change and aligns with best practices.
While the change has been made, the comment serves as a useful reminder of best practices and validates the change. It is relevant to the diff and provides context for why the change was necessary.
The comment is relevant and should be kept as it addresses a change made in the diff and reinforces best practices for styling in React components.
4. frontend/src/pages/InfraMonitoringHosts/utils.tsx:104
- Draft comment:
Avoid using inline styles. Use CSS classes or styled components instead. This applies to other similar instances in this file. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_XJ4whbNq2PhlLk7X
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> |
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 bdea4d9 in 1 minute and 0 seconds
More details
- Looked at
477
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringHosts/HostsList.tsx:131
- Draft comment:
Consider enhancing error handling to display specific error messages from the API response instead of a generic 'Something went wrong' message. - Reason this comment was not posted:
Confidence changes required:50%
TheHostsList
component inHostsList.tsx
usesisError
to conditionally render an error message. However, it doesn't handle specific error messages from the API response. Consider enhancing error handling to display more informative messages.
2. frontend/src/container/InfraMonitoringHosts/HostsList.tsx:144
- Draft comment:
Consider using a more descriptive variable name forformattedHostMetricsData
to improve readability when checking its length for conditional rendering. - Reason this comment was not posted:
Confidence changes required:50%
TheformattedHostMetricsData
is checked for length to conditionally render components. Consider using a more descriptive variable name to improve readability.
3. frontend/src/container/InfraMonitoringHosts/HostsListControls.tsx:54
- Draft comment:
If the propsshowAutoRefresh
,showRefreshText
, andhideShareModal
are set tofalse
by default inDateTimeSelectionV2
, consider omitting them to simplify the code. - Reason this comment was not posted:
Confidence changes required:50%
TheDateTimeSelectionV2
component inHostsListControls.tsx
has several props set tofalse
. If these are default values, they can be omitted to simplify the code.
4. frontend/src/container/InfraMonitoringHosts/InfraMonitoring.styles.scss:66
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values like#25e192
. This applies to other hardcoded colors in this file as well. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. frontend/src/container/InfraMonitoringHosts/InfraMonitoring.styles.scss:80
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values like#62687c
. This applies to other hardcoded colors in this file as well. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/container/InfraMonitoringHosts/InfraMonitoring.styles.scss:105
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values like#c0c1c3
. This applies to other hardcoded colors in this file as well. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/container/InfraMonitoringHosts/InfraMonitoring.styles.scss:132
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values like#fff
. This applies to other hardcoded colors in this file as well. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/container/InfraMonitoringHosts/index.tsx:1
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach. It makes it difficult to debug and find components using global search tools. - 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 the file structure, which is not something that was changed in this diff. The diff only shows changes within the file, not to the file's location or naming convention. Therefore, the comment is not about a change made in this diff.
I might be missing the context of whether the file structure was a topic of discussion elsewhere, but based on the diff alone, it doesn't seem relevant.
The task is to focus on the diff provided, and the comment does not pertain to a change made in this diff.
The comment should be deleted because it is not about a change made in this diff.
Workflow ID: wflow_hFffxO5BP1lsJbUg
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> |
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 67e1a2e in 1 minute and 24 seconds
More details
- Looked at
1314
lines of code in13
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringHosts/HostsList.tsx:150
- Draft comment:
Avoid settingdataSource
to an empty array during loading. Use theloading
prop to indicate loading state while keepingdataSource
consistent with current data. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment addresses a change made in the diff and suggests an alternative approach. However, the current implementation already uses theloading
prop to indicate the loading state, and settingdataSource
to an empty array during loading is a valid approach to prevent displaying stale data. The comment may not be necessary as the current implementation is a common pattern.
The comment might be suggesting a more efficient or cleaner approach, but without additional context or evidence that the current approach is problematic, it seems unnecessary.
The current implementation is a common pattern and does not seem to have any issues. The comment does not provide strong evidence that the current approach is incorrect.
Delete the comment as it does not provide strong evidence that the current approach is incorrect or needs change.
2. frontend/src/container/InfraMonitoringHosts/index.tsx:43
- Draft comment:
Ensure resetting query filters to an empty array on mount is necessary. It might lead to unexpected behavior if the component is re-mounted frequently. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:92
- Draft comment:
AddsetLogs
andsetHasReachedEndOfLogs
to the dependency array of thisuseEffect
to avoid stale closures. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
TheuseEffect
hook is used to update the state based on thedata
received. IncludingsetLogs
andsetHasReachedEndOfLogs
in the dependency array is not necessary because they are stable functions provided byuseState
. React guarantees that the dispatch function identity is stable and won't change on re-renders, so adding them to the dependency array is not required.
I might be missing some context about how theuseEffect
hook is used in this specific codebase, but generally, state setter functions do not need to be included in dependency arrays.
Given the general React guidelines and the context provided, it seems unlikely that including these setters in the dependency array is necessary.
The comment should be deleted because addingsetLogs
andsetHasReachedEndOfLogs
to the dependency array is unnecessary and does not align with React's guidelines for stable state setter functions.
4. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:171
- Draft comment:
Remove the eslint-disable comment for jsx-no-useless-fragment as the fragment is not useless in this context. - Reason this comment was not posted:
Confidence changes required:20%
TherenderFooter
function inHostMetricsLogs
uses a ternary operator to conditionally render elements. This is fine, but the comment to disable the eslint rule for jsx-no-useless-fragment is unnecessary since the fragment is not useless in this context.
5. frontend/src/components/HostMetricsDetail/index.tsx:263
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This issue is also present in the closeIcon prop. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
6. frontend/src/components/HostMetricsDetail/index.tsx:63
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach. It makes it difficult to debug and find components using global search tools like VS Code. This issue is also present in other components likeHostMetricsLogs
,Metrics
, andRawLogView
. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_V4eV9POOsr6QRDwB
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.
gap: 8px; | ||
padding: 12px; | ||
border-radius: 3px; | ||
border: 1px solid var(--Slate-500, #161922); |
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.
Avoid hardcoding color values. Use design tokens or predefined color constants instead. This issue is also present in other files like HostMetricsDetail.styles.scss
, HostMetricsLogs.styles.scss
, and InfraMonitoring.styles.scss
.
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 480d142 in 18 seconds
More details
- Looked at
25
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/public/locales/en-GB/titles.json:44
- Draft comment:
Ensure that the new keyINFRASTRUCTURE_MONITORING_HOSTS
is used in the application where necessary, and that it is correctly integrated with the routing and UI components. - Reason this comment was not posted:
Confidence changes required:50%
The addition of the new title key is consistent across both locale files, ensuring uniformity in the application's internationalization.
2. frontend/public/locales/en-GB/titles.json:44
- Draft comment:
The addition of "INFRASTRUCTURE_MONITORING_HOSTS" is consistent with the existing structure and correctly added in bothen-GB
anden
locale files. - Reason this comment was not posted:
Confidence changes required:0%
The changes in the JSON files are consistent and correct. The new key-value pair for "INFRASTRUCTURE_MONITORING_HOSTS" is added correctly in both files.
Workflow ID: wflow_293VXYEmbydpJqGM
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> |
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 c70d7ba in 1 minute and 1 seconds
More details
- Looked at
178
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/components/HostMetricsDetail/constants.ts:13
- Draft comment:
Ensure all references toFEATURE_COMING_SOON_STRINGS
are updated or removed to prevent runtime errors. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_FZjqF3jCGoJZNdVw
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> |
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 894e3bc in 1 minute and 2 seconds
More details
- Looked at
1178
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringHosts/InfraMonitoring.styles.scss:228
- Draft comment:
Avoid using hardcoded color values in comments. Replace#c0c1c3
with a variable for consistency. - Reason this comment was not posted:
Confidence changes required:50%
The SCSS files contain multiple instances of hardcoded color values in comments, such as#c0c1c3
and#fff
. These should be replaced with variables for consistency and maintainability.
2. frontend/src/container/InfraMonitoringHosts/InfraMonitoring.styles.scss:270
- Draft comment:
Avoid using hardcoded color values in comments. Replace#fff
with a variable for consistency. - Reason this comment was not posted:
Confidence changes required:50%
The SCSS files contain multiple instances of hardcoded color values in comments, such as#c0c1c3
and#fff
. These should be replaced with variables for consistency and maintainability.
Workflow ID: wflow_RRl2td59ZcVBO2pT
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.
placement="right" | ||
onClose={onClose} | ||
open={!!host} | ||
style={{ |
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.
Avoid using inline styles in the Drawer component. Use external stylesheets or styled components instead.
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 fce529c in 1 minute and 10 seconds
More details
- Looked at
630
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.styles.scss:33
- Draft comment:
Duplicate styles for.host-metric-traces-table .ant-table
found. Consider consolidating them to improve maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is pointing out a potential issue with maintainability due to duplicate styles. However, the styles are not exactly the same; they are likely intended for different themes (e.g., dark mode vs. light mode). This suggests that the duplication might be intentional to support different visual themes.
The comment might be missing the context that these styles are for different themes, which would justify the duplication. Without this context, the comment could be misleading.
Even if the styles are for different themes, consolidating common properties could still improve maintainability. However, the comment does not provide a clear suggestion on how to consolidate them without losing the theme-specific differences.
The comment should be deleted because the duplication is likely intentional for theme support, and the comment does not provide a clear, actionable suggestion for improvement.
2. frontend/src/components/HostMetricsDetail/HostMetricTraces/utils.tsx:5
- Draft comment:
Consider setting a default value foropenInNewTab
inBlockLink
to avoid repetitive code. - Reason this comment was not posted:
Confidence changes required:50%
TheBlockLink
component inutils.tsx
is used to create links that open in a new tab. However, theopenInNewTab
prop is not always necessary, and its default value should be set to avoid repetitive code.
3. frontend/src/container/TracesExplorer/ListView/utils.tsx:13
- Draft comment:
Consider setting a default value foropenInNewTab
inBlockLink
to avoid repetitive code. - Reason this comment was not posted:
Confidence changes required:50%
TheBlockLink
component inListView/utils.tsx
is used to create links that open in a new tab. However, theopenInNewTab
prop is not always necessary, and its default value should be set to avoid repetitive code. This is similar to the issue inHostMetricTraces/utils.tsx
.
4. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.styles.scss:108
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values likergba(255, 255, 255, 0.04)
. This applies to other instances in this file as well. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.styles.scss:189
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values likergba(0, 0, 0, 0.04)
. This applies to other instances in this file as well. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricLogs.styles.scss:105
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values like50vh
. This applies to other instances in this file as well. - Reason this comment was not posted:
Marked as duplicate.
7. frontend/src/components/HostMetricsDetail/HostMetricsLogs/NoLogsContainer.tsx:11
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values likeColor.BG_AMBER_500
. This applies to other instances in this file as well. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_aZsyBQWatcF1D3QR
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.
frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.styles.scss
Show resolved
Hide resolved
@@ -120,7 +120,6 @@ function CustomTimePicker({ | |||
|
|||
useEffect(() => { | |||
const value = getSelectedTimeRangeLabel(selectedTime, selectedValue); | |||
|
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.
remove this
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 a69f9c9 in 1 minute and 1 seconds
More details
- Looked at
36
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringHosts/utils.tsx:55
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This applies to theactive
field rendering and theProgress
component'sstrokeColor
logic. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_X8brVyOXynmuCfcX
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.
* feat: added the host list view and filters (#6210) * feat: added the host list view and filters * feat: removed group by filter and added autocomplete for where clause * feat: updated the table view and added the pagination * feat: pass updated filters to api to get filtered data in the list * feat: added global time range and order by for cpu,memory,iowait,load * feat: added order by and color codes for cpu and memory usage progress bar * refactor: removed inline styles * Host lists improvement (#6366) * style: added new style changes for date time selection in host lists view * style: added padding to date time selector * style: removed unnecessary styles for host tabs * style: removed unused css * feat: added the host detail view (#6267) * Host containers (#6297) * feat: added the host detail view * feat: completed containers and processes details view * Show host metrics panels in metrics tab. (#6306) * feat: added the host detail view * feat: completed containers and processes details view * feat: added host metrics panels in metrics tabs * refactor: removed inline styles from host containers and processes tabs * style: added top and bottom margin to containers and processes tab * Metrics time selection (#6360) * feat: added the host detail view * feat: completed containers and processes details view * feat: added host metrics panels in metrics tabs * refactor: removed inline styles from host containers and processes tabs * feat: added logs and traces tab in host metrics detail view * chore: removed console statements * feat: added DateTimeSelection component in metrics tab * style: added top and bottom margin to containers and processes tab * style: removed inline styles * feat: added logs and traces tab in host metrics detail view (#6359) * feat: added the host detail view * feat: completed containers and processes details view * feat: added host metrics panels in metrics tabs * refactor: removed inline styles from host containers and processes tabs * feat: added logs and traces tab in host metrics detail view * chore: removed console statements * feat: added filters and time selection in traces tab * fix: resolved metrics,logs and traces tab issues * feat: added navigation for logs and traces to respective explorer pages * fix: added the code for logs tab and navigation to respective explorer page * fix: added fixes for date time selection custom issue * style: added styles for light mode * refactor: removed unused code and added comments * refactor: added new code for host metric attribute keys * feat: reset query data once we are on infra monitoring page * chore: remove optional parameter from get attributes and groupby interfaces * feat: update ui as per the designs * fix: logs list, time select and other ui issues * feat: update title for infra monitoring page * feat: update copies * feat: update styles for light mode * fix: reset page size on filter, open explorers in new tab, enable horizontal scroll * feat: traces tab updates * feat: move infra monitoring behind ff * fix: remove sorting from host listing page --------- Co-authored-by: Yunus M <[email protected]> * chore: fix lint errors --------- Co-authored-by: rahulkeswani101 <[email protected]>
Summary
Created a new page InfraMonitoring. Added host list tab and completed the initial view of showing filters and global time range.
Related Issues / PR's
https://github.com/SigNoz/engineering-pod/issues/1878
Screenshots
Screen.Recording.2024-10-22.at.1.05.55.PM.mov
Affected Areas and Manually Tested Areas
Important
Add infrastructure monitoring host list view with filters, including new API endpoints, hooks, components, and routes.
InfrastructureMonitoring
component and route.InfraMonitoringHosts
withHostsList
andHostsListControls
components.InfraMonitoring.styles.scss
andInfrastructureMonitoring.styles.scss
.getHostLists
andgetInfraAttributesValues
for fetching host data and attribute values.getAttributeKeys
to support infrastructure monitoring.useGetHostList
anduseGetAggregateKeys
for fetching host lists and aggregate keys.useFetchKeysAndValues
to handle infrastructure monitoring.INFRAMONITORING_HOSTS_LIST_OPTIONS
tolocalStorage.ts
.GET_HOST_LIST
toreactQueryKeys.ts
.INFRASTRUCTURE_MONITORING_HOSTS
toroutes.ts
andpermission/index.ts
.GroupByFilter
andQueryBuilderSearch
to support infrastructure monitoring.menuItems.tsx
.This description was created by for a69f9c9. It will automatically update as commits are pushed.