Skip to content
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: add hostname and os type quick filter to hosts list #6926

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amlannandy
Copy link
Member

@amlannandy amlannandy commented Jan 27, 2025

Summary

  • Added hostname and os type quick filters to hosts list.
  • Moved hosts list table and its logic to a separate component file.

Related Issues / PR's

Fixes #6888

Screenshots

Screenshot 2025-01-27 at 1 42 32 PM

Affected Areas and Manually Tested Areas

Infra Monitoring Hosts section


Important

Add hostname and OS type quick filters to hosts list and refactor table logic into a separate component.

  • Features:
    • Add hostname and OS type quick filters to HostsList using QuickFilters component.
    • Implement HostsQuickFiltersConfig in utils.tsx for filter configuration.
  • Refactoring:
    • Move hosts list table logic from HostsList.tsx to new HostsListTable.tsx component.
  • UI/UX:
    • Update InfraMonitoring.styles.scss to style new filter and table layout.
    • Add filter visibility toggle in HostsList.tsx.

This description was created by Ellipsis for 9afdfc7. It will automatically update as commits are pushed.

@amlannandy amlannandy requested a review from YounixM as a code owner January 27, 2025 08:14
@github-actions github-actions bot added docs required enhancement New feature or request labels Jan 27, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 9afdfc7 in 1 minute and 21 seconds

More details
  • Looked at 652 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringHosts/HostsListTable.tsx:35
  • Draft comment:
    The use of useMemo for columns is unnecessary since getHostsListColumns does not depend on any props or state. Consider directly assigning the result of getHostsListColumns to columns.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of useMemo for columns in HostsListTable is unnecessary since getHostsListColumns does not depend on any props or state. This can be simplified by directly assigning the result of getHostsListColumns to columns.
2. frontend/src/container/InfraMonitoringHosts/HostsListTable.tsx:54
  • Draft comment:
    The handleTableChange function has an unnecessary dependency array for useCallback. Since it doesn't use any props or state from the component, it can be defined without useCallback.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The handleTableChange function in HostsListTable has an unnecessary dependency array for useCallback. Since it doesn't use any props or state from the component, it can be defined without useCallback.
3. frontend/src/container/InfraMonitoringHosts/HostsList.tsx:78
  • Draft comment:
    The handleFiltersChange function uses useCallback with a dependency on filters, but it also calls handleChangeQueryData, which is not included in the dependency array. Consider including handleChangeQueryData in the dependency array to avoid potential stale closures.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The handleFiltersChange function in HostsList uses useCallback with a dependency on filters. However, it also calls handleChangeQueryData, which is not included in the dependency array. This could lead to stale closures if handleChangeQueryData changes. It's better to include all used variables and functions in the dependency array.
4. frontend/src/container/InfraMonitoringHosts/utils.tsx:171
  • Draft comment:
    The HostsQuickFiltersConfig uses aggregateAttribute: 'system_cpu_load_average_15m' for both filters. Verify if this is intentional, as it might not be appropriate for the 'OS Type' filter.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The HostsQuickFiltersConfig in utils.tsx uses aggregateAttribute: 'system_cpu_load_average_15m' for both filters. This might not be correct for the 'OS Type' filter, as it seems unrelated. Verify if this is intentional or if a different attribute should be used.
5. frontend/src/container/InfraMonitoringHosts/utils.tsx:167
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values for consistency in theming.
  • Reason this comment was not posted:
    Marked as duplicate.
6. frontend/src/container/InfraMonitoringHosts/utils.tsx:3
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values for consistency in theming. This applies to the formatDataForTable function as well.
  • Reason this comment was not posted:
    Comment was on unchanged code.
7. frontend/src/container/InfraMonitoringHosts/HostsList.tsx:19
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This applies to HostsListTable and other similar components.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_2lRDExnEyFchaOtR


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs not required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add hostname and os type quick filter to hosts list
1 participant