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(frontend): critical alerts UI #4653

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

alpetric
Copy link
Contributor

@alpetric alpetric commented Nov 5, 2024

Important

Add critical alerts UI with backend support for managing alerts in settings.rs and frontend components for display and interaction.

  • Backend:
    • Add get_critical_alerts, acknowledge_critical_alert, and acknowledge_all_critical_alerts functions in settings.rs for managing critical alerts.
    • Update global_service router in settings.rs to include new routes for critical alerts.
    • Add acknowledged column to alerts table with migration scripts.
  • Frontend:
    • Add CriticalAlertButton.svelte and CriticalAlertModal.svelte for displaying and managing critical alerts in the UI.
    • Update SuperadminSettings.svelte and instanceSettings.ts to include settings for critical alerts.
    • Modify +layout.svelte to integrate critical alerts UI components and logic.

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

Copy link

cloudflare-workers-and-pages bot commented Nov 5, 2024

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: e4fa435
Status: ✅  Deploy successful!
Preview URL: https://d6b387a8.windmill.pages.dev
Branch Preview URL: https://alp-critical-alert-ui.windmill.pages.dev

View logs

alpetric and others added 11 commits November 6, 2024 14:24
…ut through a plug icon (#4652)

* Add flow prop picker

# Conflicts:
#	frontend/src/lib/components/propertyPicker/PropPicker.svelte

* fix unwanted copy

* cleaning

* Fix unset context

* move button and always display input

* fix unwanted proppicker display

* update

* update

* clean all

* clean all

---------

Co-authored-by: Ruben Fiszel <[email protected]>
@alpetric alpetric changed the title [wip] critical alerts UI feat(monitoring): critical alerts UI Nov 7, 2024
@alpetric alpetric marked this pull request as ready for review November 7, 2024 20:52
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 0d33835 in 2 minutes and 25 seconds

More details
  • Looked at 7294 lines of code in 19 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. backend/windmill-api/src/settings.rs:522
  • Draft comment:
    Consider adding a condition to the SQL query in acknowledge_all_critical_alerts to ensure only critical alerts are acknowledged, if there are different types of alerts in the system.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is speculative, as it assumes there might be different types of alerts without evidence. The current code does not show any indication of different alert types being handled differently. The comment does not provide strong evidence of an issue.
    The comment could be valid if there are indeed different types of alerts in the system, but without evidence, it remains speculative. The code does not show any filtering by alert type, which might be intentional.
    Without evidence of different alert types, the comment remains speculative. The code as it stands does not indicate a need for such a condition.
    The comment is speculative and lacks strong evidence of an issue. It should be deleted.
2. frontend/src/lib/components/sidebar/CriticalAlertButton.svelte:7
  • Draft comment:
    Typo: numUnaknowledgedCriticalAlerts should be numUnacknowledgedCriticalAlerts. This typo is present in multiple files.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In frontend/src/lib/components/sidebar/CriticalAlertButton.svelte, the variable numUnaknowledgedCriticalAlerts is misspelled. It should be numUnacknowledgedCriticalAlerts. This typo is consistent across multiple files and should be corrected for consistency and clarity.
3. frontend/src/lib/components/sidebar/CriticalAlertModal.svelte:14
  • Draft comment:
    Typo: numUnaknowledgedCriticalAlerts should be numUnacknowledgedCriticalAlerts. This typo is present in multiple files.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In frontend/src/lib/components/sidebar/CriticalAlertModal.svelte, the function updateHasUnaknowledgedCriticalAlerts has a typo in the variable name numUnaknowledgedCriticalAlerts. It should be numUnacknowledgedCriticalAlerts. This typo is consistent across multiple files and should be corrected for consistency and clarity.
4. frontend/src/lib/components/sidebar/CriticalAlertModal.svelte:121
  • Draft comment:
    Typo: numUnaknowledgedCriticalAlerts should be numUnacknowledgedCriticalAlerts. This typo is present in multiple files.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In frontend/src/lib/components/sidebar/CriticalAlertModal.svelte, the function updateHasUnaknowledgedCriticalAlerts has a typo in the variable name numUnaknowledgedCriticalAlerts. It should be numUnacknowledgedCriticalAlerts. This typo is consistent across multiple files and should be corrected for consistency and clarity.
5. frontend/src/routes/(root)/(logged)/+layout.svelte:282
  • Draft comment:
    Typo: numUnaknowledgedCriticalAlerts should be numUnacknowledgedCriticalAlerts. This typo is present in multiple files.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In frontend/src/routes/(root)/(logged)/+layout.svelte, the variable numUnaknowledgedCriticalAlerts is misspelled. It should be numUnacknowledgedCriticalAlerts. This typo is consistent across multiple files and should be corrected for consistency and clarity.

Workflow ID: wflow_f3MlcxIp7d3czdNS


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

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! Incremental review on f11d2f4 in 57 seconds

More details
  • Looked at 35 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/lib/components/propertyPicker/PropPicker.svelte:128
  • Draft comment:
    Remove commented-out code to keep the codebase clean and maintainable.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code contains commented-out sections that are not needed. These should be removed to keep the code clean and maintainable.
2. frontend/src/lib/components/propertyPicker/PropPicker.svelte:44
  • Draft comment:
    Remove unnecessary whitespace for better readability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The code contains unnecessary whitespace that should be removed for better readability and maintainability.
3. frontend/src/lib/components/flows/content/FlowLoop.svelte:133
  • Draft comment:
    Remove unnecessary backticks for better readability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The code contains unnecessary backticks that should be removed for better readability and maintainability.

Workflow ID: wflow_vKCXz0aojHE24vLw


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

@alpetric alpetric changed the title feat(monitoring): critical alerts UI feat(frontend): critical alerts UI Nov 7, 2024
@alpetric
Copy link
Contributor Author

alpetric commented Nov 7, 2024

NOTES:

  • this will enable the critical alerts UI by default (the user will have to mute in the global settings)
  • currently only the superadmin role will see the alerts

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! Incremental review on 003ec71 in 1 minute and 43 seconds

More details
  • Looked at 86 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. backend/windmill-api/src/settings.rs:520
  • Draft comment:
    Consider checking if the alert ID exists before updating and return an appropriate error if it does not exist.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. backend/windmill-api/src/settings.rs:540
  • Draft comment:
    Consider checking if any alerts were updated and return a message indicating if there were no unacknowledged alerts.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/lib/components/sidebar/CriticalAlertModal.svelte:71
  • Draft comment:
    Consider adding error handling for the acknowledgeAllCriticalAlerts service call to handle potential failures.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_tzAHdNKqA4dxvcas


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

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! Incremental review on a933af2 in 55 seconds

More details
  • Looked at 156 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. backend/windmill-common/src/utils.rs:289
  • Draft comment:
    Ensure error handling for check_mute_status is robust. Consider logging or handling the error if unwrap_or(false) is not the desired fallback behavior.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The function check_mute_status is used to determine if alerts should be muted. It fetches a boolean value from the database, which is then used to set the acknowledged field when inserting alerts. This logic seems correct and aligns with the PR description.

Workflow ID: wflow_GIqTM7SfKz8kGLms


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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants