Skip to content

Conversation

@HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Feb 14, 2024

A small post-mortem:

  • We used to check alertManagerAlerts.length > 0 || alertStackAlerts.length > 0 but the check on alertStackAlerts was removed in chore: fix cli-style useEffect exhaustive deps warnings #753.
  • So now the effect won't fire if the alertManagerAlerts is empty but the alertManagerAlerts is still populated. This is a valid use-case which @Birkbjo encountered: if there is only one alert, it cannot be removed programatically
  • This problem did not show up in the test, because we were creating a default alert, which auto-hides. So even though hiding programatically failed this did fail the test, because the alert would eventually auto-hide

I am slightly concerned that removing the condition could cause bugs, but I have no clear indication that it would. I've gone through the commit history of the Alerts.js file and the condition was there since the component started using the hook, so it's hard to say if it has been put there for a reason or if it was there purely to prevent updating state when both arrays are empty. @Birkbjo guessed it would probably be OK, so 🤞


it('can hide an alert', async () => {
const msg = 'TEST ALERT'
const options = { permanent: true }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensures the alert does't auto-hide. We need this to verify that hiding programmatically does indeed clean up the DOM.

Copy link
Member

@Birkbjo Birkbjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@sonarqubecloud
Copy link

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