Improve Interaction Handling

[Bentheburrito]

Hi @jchristgit! A month or two ago someone in the nostrum discord mentioned a design flaw with how Application Commands are run, and indeed I think I was a bit naive when I wrote this part of Nosedrum.Interactor.Dispatcher specifically:

  @impl true
  def handle_cast({:handle, %Interaction{data: %{name: name}} = interaction}, commands) do
    with {:ok, module} <- Map.fetch(commands, name) do
      response = module.command(interaction)
      Interactor.respond(interaction, response)
    end

    {:noreply, commands}
  end

Currently, if a user's command/1 callback errors, it looks like it takes down the dispatcher with it.

I think it would be better to run the body of that with inside a Task, probably using a Task.Supervisor - and more specifically Task.Supervisor.start_child/5. Although, I see in #20 you might care about knowing the success of the response, in which case maybe Task.Supervisor.async_nolink/5 would be better? What do you think? Any other approach you would recommend to make the dispatcher more resilient to crashes caused by running a user's application command?

I have some free time over the next week and could have a PR up soon if this approach looks good to you :)

[Bentheburrito]

Actually, better yet, instead of creating a Task Supervisor, maybe the caller process spawned by nostrum can run the with body by moving it to handle_interaction/1. I think we'd just have to convert that cast to a call that takes the interaction command name and returns the command module

[jchristgit]

Hello!

Actually, better yet, instead of creating a Task Supervisor, maybe the caller process spawned by nostrum can run the with body by moving it to handle_interaction/1. I think we'd just have to convert that cast to a call that takes the interaction command name and returns the command module

I think this is the preferable way to go.

Nostrum's consumer spawns a process for each gateway event, so this seems like the logical step to me. That way, the interactor is also harder to overload (for very big bots) if all it does is provide synchronized access to the storage.

Perhaps we should rename it in that case, though? What do you think?

[Bentheburrito]

Awesome! I can work on making this change then. I agree with renaming Interactor, though I'm historically a horrible namer :P Maybe we rename it to Storage and move it under the ApplicationCommand module? The full path would then be Nosedrum.ApplicationCommand.Storage. Open to thoughts about this of course.

Do you think we should rename Dispatcher too?

[jchristgit]

Awesome! I can work on making this change then. I agree with renaming Interactor, though I'm historically a horrible namer :P Maybe we rename it to Storage and move it under the ApplicationCommand module? The full path would then be Nosedrum.ApplicationCommand.Storage. Open to thoughts about this of course.

Hmm, I'm thinking that since application commands are pretty fleshed out at this point and a lot of bots are using them, making Nosedrum simpler to use for the recommended path (implementing commands with slash commands) seems logical to me, especially because application commands have all the fancy features like input validation and so on built in (a lot of nosedrum is basically just built for this).

So I think instead of moving these modules under Nosedrum.ApplicationCommand, we should move the existing text-based command handling under a module and make these the "top-level". The existing command handling could be put under Nosedrum.TextCommand or something. What do you think?

[Bentheburrito]

Ohh gotcha, that makes sense. I can definitely move the modules around in the PR then!