Skip to content

Conversation

dantovska
Copy link
Collaborator

While fixing another bug in this PR - #4339, I introduced a regression.
When user has opened the add key form but clicks on the cancel button (right next to the Add button), an toast message appears:
image

It attempts to add the empty key object, or even if there is some input for the name to the state, not considering that it may be canceled.

Applying this changes so If add key panel is closed by clicking cancel, do not pass the key name to the add key panel handler above, otherwise, include it - as is.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a regression in the "Add Key" panel so that if the user cancels the add key action, the key name is not passed to the add key panel handler.

  • Updates the closeAddKeyPanel function to separate cancel and confirm actions.
  • Ensures that panel closure and telemetry cleanup are triggered only on cancel.

@KIvanow
Copy link
Contributor

KIvanow commented Apr 15, 2025

The comments with the isCancelled right under seem a bit redundant. Other than that it looks good

Copy link
Collaborator

@pawelangelow pawelangelow left a comment

Choose a reason for hiding this comment

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

🚀

As further improvement I think we can split this callback into closePanel and closePanelAfterDataSubmit (or something better in terms of describing what actually happens). Even better would be to decouple the logic for closing panel and whatever else we do here in separate callbacks used properly. A.k.a.

closePanel () {
  onClosePanel()
  closeKeyTelemetry()
}

addKey() {
  onClosePanel()
  dispatch(data to where it has to go)
}

@dantovska dantovska merged commit 8892d00 into main Apr 16, 2025
28 checks passed
@dantovska dantovska deleted the bugfix/fix-add-key-cancel-regression branch April 16, 2025 06:43
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