Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(action): Show the error info when deleting the action failed #27124

Merged
merged 6 commits into from
Jan 6, 2025

Conversation

xu20160924
Copy link
Contributor

@xu20160924 xu20160924 commented Dec 23, 2024

Problem

Same delete API of the backend, but in the list missing shows the error information

image
image

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Handle the exceptions in the action list and display it.

Does this work well for both Cloud and self-hosted?

self-hosted worked

How did you test this code?

Self-hosted test

@xu20160924 xu20160924 changed the title fix (action) Show the error info when deleting the action failed fix(action) Show the error info when deleting the action failed Dec 23, 2024
@xu20160924 xu20160924 changed the title fix(action) Show the error info when deleting the action failed fix(action): Show the error info when deleting the action failed Dec 24, 2024
@xu20160924
Copy link
Contributor Author

@Twixes Could you please review my pull request? Thank you.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@xu20160924
Copy link
Contributor Author

waiting

@posthog-bot posthog-bot removed the stale label Jan 2, 2025
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @xu20160924!

Just one thing I noticed: It looks like there's actually a few more analogous void deleteWithUndo(...) call sites – they should all be .catch()ing potential errors, so that app behavior is consistent. Can you update those places too?

@xu20160924 xu20160924 requested a review from Twixes January 3, 2025 09:48
}).catch((e) => {
lemonToast.error(`Failed to delete hog function: ${e.detail}`)
})
Copy link
Member

Choose a reason for hiding this comment

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

Oh, thank you for going through all these! However, I wasn't clear enough, which I apologize about: We don't in fact want .catch() inside logics, since we have automatic error handling there.
This PR only needs to affect onClick handlers.
Can you quickly revert the changes to logic files? Then we'll be able to get this in immediately. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the incorrectly modified, I didn't notice the automatic error handling, I've reverted it.

@xu20160924 xu20160924 requested a review from Twixes January 3, 2025 15:00
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Awesome :shipit:

@Twixes Twixes merged commit 706f060 into PostHog:master Jan 6, 2025
97 of 98 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants