Skip to content

fix: Suppress embeds correctly #38

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jiralite
Copy link
Contributor

@Jiralite Jiralite commented Apr 8, 2025

The existing implementation is not correct.

First of all, there is no message content intent. That means you receive no embeds, and thus, this entire code path is doing nothing at all.

Unfurls in Discord are sent in the embeds array. The message content intent is required for this to populate. Furthermore, unfurls do not always happen on message create. Unfurls may take a few seconds, maybe 5 at maximum, to unfurl. When it doesn't arrive on message create, it arrives on message update.

It seems clear this line was added to attempt to fix the issue:

message = await message.fetch();

However, it's not actually fixing the issue. It just seems like it does because you're waiting for an API call to resolve. It's a coincidence.

The code has been refactored into a function that utilises both message create and message update. Also, since updating a message triggers message update, an early exit check has been added.

Important

I can see LadybirdBot does not have the message content intent enabled in the developer portal. That must be switched on for this to merge.

@Jiralite Jiralite force-pushed the fix/suppress-correctly branch from 12d948e to 5abb82b Compare April 8, 2025 12:44
@ADKaster
Copy link
Member

ADKaster commented Apr 8, 2025

The git blame pointed me to this discord message: https://discord.com/channels/830522505605283862/830739873119207426/986027015499022386

Which has this video embedded in it from @networkException

2022-06-13_23-58-00.mp4

I added the meesage content intent to my test bot in the developer portal:

image

Which fixed a 'used disallowed intents' error on your branch here, as expected.

But when pasting a link to a github issue for ladybirdbrowser/ladybird in my test guild, the bot did not "delete the embed" as was shown in the earlier video. Am I misunderstanding the intent of the code here?

@Jiralite
Copy link
Contributor Author

Jiralite commented Apr 8, 2025

But when pasting a link to a github issue for ladybirdbrowser/ladybird in my test guild, the bot did not "delete the embed" as was shown in the earlier video. Am I misunderstanding the intent of the code here?

Send what link you tested with. If you check the code, not all links are suppressed.

Additionally, no code changes have been made to this, so this would not be in scope of this pull request, but I can help out regardless.

@ADKaster
Copy link
Member

ADKaster commented Apr 8, 2025

Oh now that is curious. I didn't realize it was intended to only suppress embeds for linked code (i.e. repo/blob/branch/<filepath>.

It still fails though, probably missing a "delete embed" permission in my token :)

image

@Jiralite
Copy link
Contributor Author

Jiralite commented Apr 8, 2025

Manage messages is required for this. It wasn't checked previously, but at least now it will not error.

@ADKaster
Copy link
Member

ADKaster commented Apr 8, 2025

Thanks! It works after re-adding the bot to my test server with that bot permission added.

@Jiralite
Copy link
Contributor Author

Jiralite commented Apr 8, 2025

The git blame pointed me to this discord message: discord.com/channels/830522505605283862/830739873119207426/986027015499022386

I cannot see this, maybe it's in a private channel? Anyway, I would refer to the third paragraph in my first comment here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants