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

Interaction already acknowledged when using two interactable messages #868

Closed
OdedItkinOW opened this issue Sep 15, 2022 · 9 comments · Fixed by #973
Closed

Interaction already acknowledged when using two interactable messages #868

OdedItkinOW opened this issue Sep 15, 2022 · 9 comments · Fixed by #973
Labels
bug Something isn't working enhancement New feature or request

Comments

@OdedItkinOW
Copy link

Describe the bug
I have a simple /test command, that creates a simple message with a few buttons and a select menu.
I have a simple collector used by that command, that calls interaction.update() on the original message whenever any button is interacted with.

If I only run /test once, everything works fine.
However, if I run /test twice, without closing the collector in between (and hence have two messages that i'm supposed to collect from), any interaction with any of the collected messages (or any other one left from previous builds for that matter), will cause both collectors to receive the same interaction, meaning that the second one will throw:
DiscordAPIError[40060]: Interaction has already been acknowledged error to be thrown

To Reproduce
Steps to reproduce the behavior:

  1. run /test
  2. run /test again
  3. click any button on any interactable message
  4. "interaction has already been acknowledged" error is thrown

Expected behavior
No double-creation of collectors. Or if they are doubly created, then a way to distinguish them between their target messages.

Desktop (please complete the following information):

  • OS: Windows 11
  • Browser: N/A
  • Version: 4.3.0 discord-nestjs
@fjodor-rybakov
Copy link
Owner

Can you provide repo with reproduce?

@OdedItkinOW
Copy link
Author

OdedItkinOW commented Sep 15, 2022

Not really, but it's a generated nestjs project with three classes that were changed (two services and a module, the module being loaded by the main thing), which are the following:

on-start.module.ts:

@Module({
  imports: [
    DiscordModule.forRootAsync({
        useFactory: () => ({
            token: '${discord.token}',
            discordClientOptions: {
                intents: [GatewayIntentBits.Guilds]
            },
            autoLogin: true
        })
    })
  ],
  providers: [OnStartService]
})
export class OnStartModule {}

on-start.service.ts

@Command({
    name: 'test',
    description: 'helloooooooo'
})
@UseCollectors(CollectorService)
@Injectable()
export class OnStartService implements DiscordCommand {
    handler(interaction: CommandInteraction): InteractionReplyOptions {
        const back = new ActionRowBuilder<ButtonBuilder>()
        
        .addComponents(
            new ButtonBuilder()
                .setCustomId('1')
                .setLabel('1')
                .setStyle(ButtonStyle.Primary)
            ,
            new ButtonBuilder()
                .setLabel('2')
                .setStyle(ButtonStyle.Link)
                .setURL('https://google.com')
            ,
            new ButtonBuilder()
                .setCustomId('3')
                .setLabel('3')
                .setStyle(ButtonStyle.Danger)
            ,
            new ButtonBuilder()
                .setCustomId('4')
                .setLabel('4')
                .setStyle(ButtonStyle.Secondary)
            ,
            new ButtonBuilder()
                .setCustomId('5')
                .setLabel('5')
                .setStyle(ButtonStyle.Success)
        );
        const dropdown = new ActionRowBuilder<SelectMenuBuilder>()
            .addComponents(
                new SelectMenuBuilder()
                    .setCustomId('select')
                    .setPlaceholder('Nothing selected')
                    .addOptions(
                        {
                            label: 'Select me',
                            description: 'This is a description',
                            value: 'hello',
                        },
                        {
                            label: 'You can select me too',
                            description: 'This is also a description',
                            value: 'second_option',
                        },
                    )
            )

        return { ephemeral: true, content: 'Pong!', components: [dropdown, back] };
    }
}

collector.service.ts

@InteractionEventCollector({
})
@Injectable({ scope: Scope.REQUEST })
export class CollectorService {
    constructor(
        @InjectCollector()
        private readonly collector: InteractionCollector<ButtonInteraction>,
      ) {
        console.log("created", collector) // this runs twice at project setup, and once per command invocation
      }
    
    @On('collect')
    async onCollect(interaction: ButtonInteraction): Promise<void> {
        await interaction.update({
            content: `A button was clicked! Specifically: ${interaction["values"] ? interaction["values"][0] : interaction["customId"]}`,
            components: [],
        });
        this.collector.stop();
        return;
    }
    
    @Once('end')
    onEnd(): void {
      console.log('end');
    }
}

@OdedItkinOW
Copy link
Author

Update:
After further debugging the issue, it seems like it stems from the fact that every message creates a collector, but the collectors themselves are not filtered on a per-message basis. This in turn simply means that every collector subscribes to all events from that channel.

It seems that the simplest solution would most likely be for me to pass a filter to each collector, only allowing it to interact with the message created by the given command. However, I'm currently uncertain of how exactly that could be achieved. I'm looking into the code further, and will update if I find a clean method of doing it

@Phoscur
Copy link

Phoscur commented Sep 18, 2022

Hey, sorry I don't want to hijack this thread, but I think it shows that a more complete example would be helpful and I think the reproduction is related to what I am requesting here: #869

@OdedItkinOW thanks for showing some example code with a MenuBuilder ;)

@OdedItkinOW
Copy link
Author

OdedItkinOW commented Sep 18, 2022

Final update I think:
It is possible to achieve this behavior by slightly circumventing the API. Specifically, if we go to on-start.service.ts, rather than directly returning the message components, it is possible to just add the following:

        const reply = await interaction.reply({ ephemeral: true, content: 'Pong!', components: [dropdown, back] });
        const collector = reply.createMessageComponentCollector()

And then to simply remove the UseCollectors() annotation.

Then, collector itself can be used to collect the interactions from this individual message. This works. But it's a "pure" discord.js solution, rather than properly utilizing discord-nestjs itself, and requires a partial re-implementation of discord-nestjs functionality, since it doesn't seem like this raw discord.js collector can then be cleanly inserted into the UseCollectors() annotation's flow.

I'll probably be using this at least for now, since my implementation is somewhat time-sensitive, but leaving this issue here and opened since I feel it is important, and would love to move to a cleaner approach in the future when possible.

edit:
it does however seem, that if it was possible to, in this particular line, add some logic to allow reply.createMessageComponentCollector() to be ran instead, it would work as a fix

edit 2:
I started actually digging into it. Collector creation is currently done before the reply to a command is sent, so this approach is invalid. After discussing it further with a colleague, I decided to try and see if I can create a clean pull request that implements this feature

@Phoscur
Copy link

Phoscur commented Sep 18, 2022

discord-nestjs\node_modules@discordjs\rest\src\lib\handlers\SequentialHandler.ts:287
this.manager.globalReset = Date.now() + 1000;
^
DiscordAPIError[40060]: Interaction has already been acknowledged.
at SequentialHandler.runRequest (\discord-nestjs\node_modules@discordjs\rest\src\lib\handlers\SequentialHandler.ts:287:15) [...]
at PostInteractionCollector.onCollect (\discord-nestjs\packages\sample\interaction-collector\src\bot\post-interaction-collector.ts:8:5)

@fjodor-rybakov Reproduction within this repo in sample/interaction-collector:
Run /song xyzmeh twice, then try to "play" both
grafik

@Phoscur
Copy link

Phoscur commented Sep 18, 2022

@OdedItkinOW great to read that you want to provide a PR, not sure if I can help but I would like to! (Maybe let me have a look at your WIP for code review?)

I am still struggling with discord.js fundamentals... When the bot sample/interaction-collector is restarted, it won't "play" from previously sent commands before the restart. How to set up a collector which keeps collecting and never acknowledges the interaction?

@OdedItkinOW
Copy link
Author

I'm still looking at the source code, I'll see about notifying you if and when I do start working on the pr.

As for the other question, I can answer that, but that's off-topic for the issue, so I'd rather move it somewhere else to avoid clutter

@fjodor-rybakov fjodor-rybakov added bug Something isn't working enhancement New feature or request labels Jan 27, 2023
@fjodor-rybakov fjodor-rybakov linked a pull request Jan 29, 2023 that will close this issue
@fjodor-rybakov
Copy link
Owner

In new version it will be possible to inject the event with which the collector was created. In @Filter() method you can filter the message so that the collector is attached to the original message. In this case, the error will no longer appear.

@Injectable({ scope: Scope.REQUEST })
@InteractionEventCollector({ time: 15000 })
export class PostInteractionCollector {
  constructor(
    @InjectCauseEvent()
    private readonly causeInteraction: ChatInputCommandInteraction,
  ) {}

  @Filter()
  filter(interaction: ButtonInteraction): boolean {
    return this.causeInteraction.id === interaction.message.interaction.id;
  }

  @On('collect')
  async onCollect(interaction: ButtonInteraction): Promise<void> {
    await interaction.update({
      content: 'A button was clicked!',
      components: [],
    });
  }

  @Once('end')
  onEnd(): void {
    console.log('end');
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants