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

Allow using Discord threads #3362

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

Conversation

A5rocks
Copy link

@A5rocks A5rocks commented Feb 17, 2025

Absolutely hacky support for Discord threads. There's a couple important details about Discord threads:

  • they do not support topics
  • (a bit less importantly, they can't be marked NSFW nor have overwrites)

The lack of topic support is troubling given that Modmail stuffs thread information into the channel topic. I updated some things to work based off the genesis message and -- since that does not contain the title -- disallowed setting titles for thread in Discord threads.

This can be used by setting main_category_id to a text channel, rather than a category channel. It worked in some basic tests I did locally, but I haven't checked in a more intensive environment.

@martinbndr
Copy link
Contributor

This would be a nice idea, but this completely overwrites the whole system to threads which can be a problem. There are servers that would like to use the old/original behaiviour from the current modmail. So to have a chance for being considered it would be better to implement this as an optional feature with a configuration variable.

@lorenzo132
Copy link
Member

This would be a nice idea, but this completely overwrites the whole system to threads which can be a problem. There are servers that would like to use the old/original behaiviour from the current modmail. So to have a chance for being considered it would be better to implement this as an optional feature with a configuration variable.

I agree with this. If it were to be made a configurable option and thoroughly tested in an intensive environment, I’d be 100% okay with merging this.

@A5rocks
Copy link
Author

A5rocks commented Feb 17, 2025

This still has the old behavior -- it only uses threads if main_category_id is a text channel. (At least it should work with a category; I only tested with None and with a text channel.)

@martinbndr
Copy link
Contributor

Ah okay

@martinbndr
Copy link
Contributor

martinbndr commented Feb 17, 2025

I have not tested everything yet but a few things I have noticed:

  • During the bot tries to create a modmail thread it´ll fails with this error:
await channel.edit(topic=f"User ID: {user_id}")
TypeError: Thread.edit() got an unexpected keyword argument 'topic'
  • Bot logging shows:
    02/18/25 00:02:18 core.thread[1339] - WARNING: Found existing thread for 618805150756110336 but the channel is invalid.
    So there are these and may more parts that still does not fully recognize the main_category_d is a text channel

And may it would be better to let user configure it sth else. Because the variable name main_categpory_id would be a bit confusing for the users as they are thinking only categories are valid.

@A5rocks
Copy link
Author

A5rocks commented Feb 18, 2025

I'm unable to reproduce either of those -- do you have some steps I should follow? (I can see where in the code they happen and could blindly put guards up, but I want to know in what case they happen)

@martinbndr
Copy link
Contributor

martinbndr commented Feb 19, 2025

I just used the config set command and configured the main_category_id to a text channel.
and then dmed the bot to start a new thread.

Copy link
Contributor

@martinbndr martinbndr left a comment

Choose a reason for hiding this comment

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

Line 1305/1322 (including your changes) could be the issue. Also wondering why you didnt get this error. Have you tested it properly?

@A5rocks
Copy link
Author

A5rocks commented Feb 19, 2025

I've been testing repeating:

  1. Set main category id to a category
  2. Create a thread
  3. Close it
  4. Switch main category id to a text channel
  5. Create a thread
  6. Close it

Obviously with stuff like checking switching 3 and 4. Nothing gets logged or breaks.

I'll try looking at the code some more and see if I can see a way for maybe a .find call to race against sending a genesis message?

@A5rocks
Copy link
Author

A5rocks commented Feb 19, 2025

I took a wild guess -- let me know if that fixes it (I am still unable to repro...)

(for people skimming this PR: I changed modmail threads to cache only when completely initialized, including their first message)

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.

3 participants