kord-extensions icon indicating copy to clipboard operation
kord-extensions copied to clipboard

Component callbacks do not work properly

Open Tmpod opened this issue 3 years ago • 5 comments

Description

If you register a component callback with the Koin-provided ComponentCallbackRegistry, components are not registered using the given id string, and request's custom_id field is populated with a UUID, as seen in the following log sample:

15:37:23.229 [DefaultDispatcher-worker-6] TRACE c.k.k.e.components.ComponentRegistry - Registering component with ID: b7bba28d-c137-46bc-a26a-b438834a335e
15:37:23.241 [DefaultDispatcher-worker-6] DEBUG [R]:[KTOR]:[ExclusionRequestRateLimiter] - [REQUEST]:POST:/webhooks/{application.id}/{interaction.token} params:[{application.id}=...,{interaction.token}=...] body:{"embeds":[],"components":[{"type":1,"components":[{"type":2,"style":1,"label":"foo","custom_id":"b7bba28d-c137-46bc-a26a-b438834a335e","disabled":false}]}]}

This results in a message like Button interaction received for unknown component ID: b7bba28d-c137-46bc-a26a-b438834a335e each time the component is interacted with.

Versions

I'm using KordEx 1.5.5-SNAPSHOT (with Kord 0.8.x-SNAPSHOT)

Code Examples

The code used for the above example is as follows:

// inside an extension's setup method
val btnName = "foobtn"
get<ComponentCallbackRegistry>().registerForPublicButton(btnName) {
    action {
        respondEphemeral { content = "foo" }
    }
}
publicSlashCommand {
    name = "foo"
    description = "foo"
    action {
        respond {
            components {
                publicButton {
                    label = "foo"
                    useCallback(btnName)
                }
            }
        }
    }
}

Suggestions

When receiving interaction events (registered on ExtensibleBot), more concretely in these two ComponentRegistry#handle methods, you only lookup the unique componentId and immediately say it is unknown without checking custom IDs.

When registering components, well, I can't really find where you give components a custom ID.

Tmpod avatar Aug 26 '22 15:08 Tmpod

Hello, and thanks for opening an issue! As this is the first time you've created an issue on this repository, we'd just like to offer you a warm welcome to the project, and the following pointers:

  • Most importantly, all issues must adhere to our Code of Conduct. Please give it a quick read if you haven't already.

  • While our team is passionate about the projects we've created here, we're all volunteers. Please don't be offended if it takes time for us to get to your issue - we'll be here as soonas we can be!

  • Please provide as much information as possible when asking a question, reporting a problem, or submitting a feature request. This will help us to address your issue quickly and efficiently. If you forgot to add some information, no worries - feel free to edit the issue and add anything you missed!

    Thanks for contacting us! If you have any further questions, please feel free to join us on Discord in the #dev-kotdis channel (or #kordex-discussion for Kord Extensions projects), or to contact a staff member directly.

boring-cyborg[bot] avatar Aug 26 '22 15:08 boring-cyborg[bot]

we'll be here as soonas we can be!

missing a space there ;)

Tmpod avatar Aug 26 '22 15:08 Tmpod

Having looked at the code, I see no reason for this to happen - buttons registered with a callback via useCallback simply have their action set as they normally would do, it's just that the action and check blocks attempt to grab the callback and use it.

If you have time, can you throw a debugger at this and check that the components are being registered in the component registry? They definitely should be, since… they work, but still.

gdude2002 avatar Sep 25 '22 09:09 gdude2002

Having looked at the code, I see no reason for this to happen - buttons registered with a callback via useCallback simply have their action set as they normally would do, it's just that the action and check blocks attempt to grab the callback and use it.

Right, but the thing is that the "grabbing" of such callback is only done by the unique interaction ID, instead of the custom_id field, which I'd assume is what should be used here?

If you have time, can you throw a debugger at this and check that the components are being registered in the component registry? They definitely should be, since… they work, but still.

I will do that sometime this week, but like, does this work for you? Can anybody else reproduce this behaviour?

Tmpod avatar Sep 25 '22 22:09 Tmpod

buttons registered with a callback via useCallback simply have their action set as they normally would do, it's just that the action and check blocks attempt to grab the callback and use it.

Right, but that isn't the issue. The issue is that the component isn't even invoked because it's supposedly "unknown", and that happens because you only check for the unique ID and not for the custom ID (which is what allows cross-session components to work). See this.


Edit:

Mmmm, actually, you do get the custom ID because ComponentInteraction#componentId gets the custom ID. The issue is actually the component registering, which isn't done using the custom ID provided in the callback; for example, here.

Tmpod avatar Oct 02 '22 18:10 Tmpod

This system is no longer supported due to limitations in Kotlin's type system, and it's been removed in 1.5.6-SNAPSHOT.

The system as written was very memory-intensive, and was very much not a good approach to solving this problem. Please handle the event yourself instead, and define components manually using predictable customId properties. Remember that when you're handling the events yourself, you can match partial component IDs and store other identifying or random data alongside the part of the string you're matching!

gdude2002 avatar Dec 15 '22 21:12 gdude2002

Well, this was quite good timing, as I just started working on this bot again and was soon going to tackle this issue.

It's actually pretty easy, I'm not sure why I didn't think of this first. The old CallbackRegistry really doesn't need to exist.

At the moment, I just fix the component's id to some value, in the builder, and then register the component (with ComponentRegistry::register, instance gotten from Koin) in my extension setup and it's good to go. Since this only happens once in the program's lifetime, there shouldn't be a problem.

I'm still curious though, what made this so memory intensive?

Tmpod avatar Dec 21 '22 19:12 Tmpod

It was memory-intensive because it required manually re-registering component handlers for every single individual component you wanted to persist, which, well, gets to be a lot over time if you have a busy bot.

There was no need for it to be totally honest - you can achieve the same thing and still provide a random value in the component ID if you just parse the IDs using some kind of separator and a consistent naming convention.

gdude2002 avatar Dec 22 '22 15:12 gdude2002