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

chore(slack): Remove redundant thread ID generation #1910

Conversation

0xRichardH
Copy link
Contributor

@0xRichardH 0xRichardH commented Feb 19, 2025

Thread ID Generation Inconsistency:

There are two different thread ID generation approaches in the handle_message function. The first one is immediately overwritten by the second:

# backend/chainlit/slack/app.py

- thread_id = str(
-     uuid.uuid5(
-         uuid.NAMESPACE_DNS,
-         message["channel"] + datetime.today().strftime("%Y-%m-%d"),
-     )
- )
 thread_ts = message.get("thread_ts", message["ts"])
 thread_id = str(uuid.uuid5(uuid.NAMESPACE_DNS, thread_ts))

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Feb 19, 2025
uuid.NAMESPACE_DNS,
message["channel"] + datetime.today().strftime("%Y-%m-%d"),
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to keep the per day segmentation? If not this means that we will get one thread per channel which can grow pretty fast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thread_ts is a thread message key, which means we will get one chainlit thread per Slack thread.

https://api.slack.com/messaging/retrieving
image

Copy link
Collaborator

@willydouhard willydouhard left a comment

Choose a reason for hiding this comment

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

LGTM

@willydouhard willydouhard added this pull request to the merge queue Mar 7, 2025
Merged via the queue into Chainlit:main with commit a599538 Mar 7, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants