search

mattermost / mattermost-plugin-google-calendar

Mattermost Google Calendar Plugin
35 stars 35 forks source link

[MM-55526] Fix slash command failure when issued from Calls thread #65

Closed streamer45 closed 10 months ago

streamer45 commented 10 months ago

Summary

Some slash commands are not working when issued from the Calls chat thread (in the pop out window). I thought it was a Calls issue but locally it didn't happen so I debugged a bit on Community and the exception is actually coming from this plugin (see screenshot). The returned providerConfiguration can be null which will cause an exception and break the whole chain of slash handlers.

image

Ticket Link

https://mattermost.atlassian.net/browse/MM-55526

mickmister commented 10 months ago

I'm thinking the webapp code that runs the hooks (e.g. slashCommandWillBePosted) should be made more resilient, so when errors like this happen, it doesn't bubble up and cause short-circuiting like this. In this case, it's a bit of undefined behavior, because this is a "plugin may want to short-circuit and handle the operation", rather than a pure "fire and forget" hook.

Either way, I'm thinking there should be some error-handling around calling these hooks, as we should assume that an error could happen in any of these plugin hooks.

streamer45 commented 10 months ago

@mickmister Agreed :100: Would you mind filing a ticket? I think it could be a very good improvement.

mickmister commented 10 months ago

@streamer45 There are two related tickets I created in the past that cover this. There was some work to introduce error boundaries for the plugin React components (though not merged), and the smaller-scoped work for the function hooks like slashCommandWillBePosted got lost in the seams of things.

https://github.com/mattermost/mattermost-webapp/pull/11467 https://mattermost.atlassian.net/browse/MM-42330 - mentions handling hooks like slashCommandWillBePosted https://mattermost.atlassian.net/browse/MM-42329

streamer45 commented 10 months ago

Thanks for the extra context. It looks like the work was ready to be merged, pending QA. Wondering how much effort it would be to rebase it now. It's a pity to see such a good effort go wasted.