Kord-Extensions / kord-extensions

Kord extensions framework, providing commands and distinct units of functionality
https://kordex.dev
European Union Public License 1.2
112 stars 27 forks source link

Component callbacks do not work properly #180

Closed Tmpod closed 1 year ago

Tmpod commented 2 years ago

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.

boring-cyborg[bot] commented 2 years ago

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:

Tmpod commented 2 years ago

we'll be here as soonas we can be!

missing a space there ;)

gdude2002 commented 2 years ago

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.

Tmpod commented 2 years ago

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 commented 2 years ago

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.

gdude2002 commented 1 year ago

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!

Tmpod commented 1 year ago

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?

gdude2002 commented 1 year ago

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.