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

add screenshot support for Slack Handler #18

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

ramaarf
Copy link

@ramaarf ramaarf commented Jan 17, 2024

I've add screenshot support for Slack Handler, the user need to provide api token to access Slack Api files.upload

@ThexXTURBOXx
Copy link
Owner

Thank you very much for your PR! Have you tested this somehow?
Also, there might be the possibility for a race condition: If two users crash at (almost) the same time, the error message of user A will appear and it will start uploading the screenshot, but in the mean time the error message of user B will appear and he will also start uploading. Only after that the screenshots will follow - also in some random order which is dependent on the internet connection of user A and B.
The worst possible case would be the following order:

Error message of user A
Error message of user B
Screenshot of user B
Screenshot of user A

Is it somehow possible to send a message with a screenshot in a single API call such that the screenshot is linked to the given error message?

@ThexXTURBOXx
Copy link
Owner

Here it looks like they are additionally uploading an image with the message. Maybe that would be a better approach?

@ramaarf
Copy link
Author

ramaarf commented Jan 18, 2024

Afaik, to upload file on slack we need to call it separately via https://slack.com/api/files.upload, the attachment only work with image url not uploading with the message. So I've made an update by changing the flow to upload the screenshot first to the channel, then linked the url to the error message as attachment.

@ThexXTURBOXx ThexXTURBOXx merged commit e14ab21 into ThexXTURBOXx:master Jan 22, 2024
@ThexXTURBOXx
Copy link
Owner

I have tried a few things and could not get it to work any better than that. So, I will publish it soon. Thanks for the contribution! :)

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.

None yet

2 participants