Remora / Remora.Discord

A data-oriented C# Discord library, focused on high-performance concurrency and robust design.
GNU Lesser General Public License v3.0
243 stars 46 forks source link

[Bug] FeedbackService `SendAsync` does not pass message flags from `options` to `CreateMessage` #330

Closed lyssieth closed 5 months ago

lyssieth commented 5 months ago

Description

The function SendAsync doesn't pass any of the message flags in the options.MessageFlags to the underlying CreateMessage call.

The issue happens in https://github.com/Remora/Remora.Discord/blob/a1169a7b5c2817140e5287dff750bbaffa298114/Remora.Discord.Commands/Feedback/Services/FeedbackService.cs#L369-L389, where the flags simply aren't passed to CreateMessageAsync, even though I'm pretty sure it should pass the flags there.

I'm trying to do the following:

await feedback.SendContextualInfoAsync(
        "A message here",
        authorId,
        new() { MessageFlags = MessageFlags.Ephemeral, },
        CancellationToken
      );

But the Ephemeral flag is being completely ignored.

Steps to Reproduce

  1. Try to send a message with FeedbackMessageOptions.MessageFlags = MessageFlags.Ephemeral

Expected Behavior

Sending an ephemeral message

Current Behavior

Completely ignoring the flag

Library / Runtime Information

Remora ver. 2024.1.0 Dotnet ver. 8.0.203

Nihlus commented 5 months ago

I don't believe Discord supports sending ephemeral messages as anything except followup messages to interactions; at least, that was the case when this code was written. Discord's FAQ regarding ephemeral messages suggests that's still the case.

If you look further down into SendContextualAsync you'll see some special handling for ephemeral messages - you can flag your command with the Ephemeral attribute if you want your whole slash command to be ephemeral, or add the flag (as you are doing) to individual messages when in a context that supports them. If you're not, the flag is silently ignored as you've noticed.

lyssieth commented 5 months ago

Well, this is in an interaction/slash command context, so theoretically it should be valid to reply with an ephemeral message. I don't want to set the whole thing as ephemeral, as the ephemeral message is only supposed to be for the "help" part of the command. I might have to just factor that into a separate command.

I would also like to put contextual errors as ephemeral messages, for the purpose of not spamming chat with errors. Is there a better solution than just having my own custom FeedbackService?

VelvetToroyashi commented 5 months ago

Well technically Nihlus is correct, for the most part, though I don't think it's unreasonable to expect the feedback service to send a followup instead of a message if the current context is an interaction, since this is the way Discord would want you to do it anyway, unless you're sending a Direct Message to the user. Some work was already put into improving the feedback service in this regard, so I think this is a good step forward.

Nihlus commented 5 months ago

SendAsync is explicitly meant to not send a followup - it's a wrapper for sending a normal message. If you're ending up in that function, you're not in an interaction context (or we have another, deeper bug). FeedbackService will send a followup instead of a normal message if you are in an interaction context.

Could you check that you are actually calling SendContextualInfoAsync in a situation where FeedbackService's _contextInjection.Context is an InteractionContext?

lyssieth commented 5 months ago

It's an InteractionCommandContext, which... should be (for this case) practically equivalent to InteractionContext as far as I understand it?

I put a breakpoint on switch (_contextInjection.Context) of SendContextualAsync, and examined it a bit above. an image showcasing the debugger evaluation of `_contextInjection.Context`

Nihlus commented 5 months ago

Alright, in that case, you just need to add the Ephemeral attribute to your command method. I leafed through the code again, and remembered that Discord requires you to flag the initial interaction as ephemeral to enable it for any followup.

If the first, original message you send is ephemeral, then all subsequent ones must also be ephemeral (hence the code on line 419); that, however, should not preclude you from sending a mix of ephemeral and non-ephemeral messages as long as the first message you send as a response is not ephemeral.

lyssieth commented 5 months ago

Discord requires you to flag the initial interaction as ephemeral to enable it for any followup

Is the default behavior of Remora to defer the response, thus why it needs the Ephemeral attribute there?

Because given how it's set up right now, my SendContextualInfoAsync should be the first response to the original message and thus fine being Ephemeral?

But if it's default behavior to defer first and then execute the command, that'd be nice to either be a documented default (unless I'm blind in which case sorry!) rather than somewhat hidden.

Nihlus commented 5 months ago

Yeah, the default behaviour is to defer and then execute. The docs could probably be a bit clearer, but it's stated here: https://github.com/Remora/Remora.Discord/tree/main/Remora.Discord.Commands#suppressinteractionresponse

lyssieth commented 5 months ago

Thanks for the clarification! I definitely missed that, but also could be clearer in the first place.