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

fix: remove potentially erroneous check #10068

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

monbrey
Copy link
Member

@monbrey monbrey commented Jan 3, 2024

In fixing a bug, #8598 seems to have introduced a different bug in which the InteractionCollectors checks would incorrectly reject interactions coming from MessageComponents in a reply to other MessageComponentInteractions.

Removing this check seems to resolve the issue, and I believe I've tested the original scenario too. However, I'm not certain so would appreciate feedback from @RedGuy12 and @ImRodry who fixed/raised the original issue.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

@monbrey monbrey requested a review from a team as a code owner January 3, 2024 00:51
Copy link

vercel bot commented Jan 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Jan 3, 2024 0:52am
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Jan 3, 2024 0:52am

@ImRodry
Copy link
Contributor

ImRodry commented Jan 3, 2024

I have a feeling this shouldn't work just by looking at the code, but my tests do pass and I don't think there was anything more complicated than the test provided in the initial issue, so it's fine by me. Maybe RedGuy can explain why he added that line though

@monbrey
Copy link
Member Author

monbrey commented Jan 3, 2024

I have a feeling this shouldn't work just by looking at the code, but my tests do pass and I don't think there was anything more complicated than the test provided in the initial issue, so it's fine by me. Maybe RedGuy can explain why he added that line though

Exactly why I pinged you both for a sanity check 😄 Seemed too easy to be right.

@RedGuy12
Copy link
Contributor

RedGuy12 commented Jan 6, 2024

Maybe RedGuy can explain why he added that line though

Honestly can't remember, it does look like it should work with it tho /shrug

@Apokalypt
Copy link
Contributor

Do not merge this change! If you do that you will have other problems 😅 Here is a simple POC of the problem:

require('dotenv').config();

const { Client, GatewayIntentBits, ButtonBuilder, ButtonStyle, ComponentType, MessageFlags } = require('discord.js');

const client = new Client({ intents: [GatewayIntentBits.Guilds, GatewayIntentBits.GuildMessages, GatewayIntentBits.MessageContent] });

client.once('ready', () => {
    console.log('Bot is ready!');
});
client.on('messageCreate', (message) => {
    if (message.author.bot) {
        return;
    }

    if (message.content === 'ping') {
        const button = new ButtonBuilder()
            .setStyle(ButtonStyle.Primary)
            .setLabel('Click me!')
            .setCustomId('click_me');

        message.reply({ content: 'pong', components: [{ type: ComponentType.ActionRow, components: [button] }] });
    }
});

let awaitMessageComponent = false;
client.on('interactionCreate', async (interaction) => {
    if (!interaction.isMessageComponent()) {
        // Type guard
        return;
    }

    if (awaitMessageComponent) {
        // Skip when we are awaiting to show the bug (collect all component interaction)
        return;
    }

    if (interaction.message.flags.has(MessageFlags.Ephemeral)) {
        const button = new ButtonBuilder()
            .setStyle(ButtonStyle.Danger)
            .setLabel('DO NOT CLICK!')
            .setCustomId('click_me_await');

        const msg = await interaction.update({
            content: 'Do not click the button below! Click again on the button above!',
            components: [{ type: ComponentType.ActionRow, components: [button] }]
        });

        awaitMessageComponent = true;
        await msg.awaitMessageComponent({ componentType: ComponentType.Button, time: 10_000 })
            .then( i => {
                i.reply({ content: 'Component collected!', ephemeral: true });
            })
            .catch( e => {
                console.error(e);
            });
        awaitMessageComponent = false;
    } else {
        // To encounter the bug we need to call "update" on an ephemeral message in order to receive, as the return
        // of the call, an InteractionResponse instance and then call "awaitMessageComponent" on it.

        const button = new ButtonBuilder()
            .setStyle(ButtonStyle.Danger)
            .setLabel('Click this one!')
            .setCustomId('click_me');

        interaction.reply({
            content: interaction.message.content === 'pong' ? 'Click this one please' : 'Click the button below!',
            components: [{ type: ComponentType.ActionRow, components: [button] }],
            ephemeral: true
        });
    }
});

client.login(process.env.BOT_TOKEN);

You'll be able to understand and run the bot easily, I've got to go to work so I can't do anything more detailed at the moment. If you need something don't hesitate to ask (I was working on a fix because I've seen the same bug today while putting up to date discord.js)

@@ -40,7 +40,7 @@ class InteractionCollector extends Collector {
* The message from which to collect interactions, if provided
* @type {?Snowflake}
*/
this.messageId = options.message?.id ?? options.interactionResponse?.interaction.message?.id ?? null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this condition will lead to invalid interaction collected when we want to collect component interaction after the call of the "update" function on an ephemeral message
(see my long comment with a POC)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I suspected there would be an edge case this would miss. Will keep looking into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants