Skip to content

RFC: Slack thread support#524

Merged
GitOnUp merged 17 commits intomainfrom
slack-thread-support
Aug 24, 2023
Merged

RFC: Slack thread support#524
GitOnUp merged 17 commits intomainfrom
slack-thread-support

Conversation

@GitOnUp
Copy link
Copy Markdown
Contributor

@GitOnUp GitOnUp commented Aug 21, 2023

Adds knowledge of threading to the Slack Transport.

In its current form, the transport will respond to the channel ID only, which means that if you respond from a thread the transport will respond outside of it. This adds knowledge of the thread_ts field to blocks, which is the ts of the parent message in the thread, and is specified in the chat.postMessage method as a way to reply to it.

Some open questions about whether context ID for the bot context should be channel or (channel,thread) tuple.

I've also noticed that the Tags representing slack metadata on those blocks seems to be getting stripped on its way down to the tool, which I'm assuming is defined behavior but I haven't traced it yet.

thread_ts = get_block_thread_ts(incoming_message)
# TODO (george) not sure this is always what we want, unfortunately. Some bots may want to keep a
# conversation going through threads, others may not?
context_id = chat_id if not thread_ts else f"{chat_id}-{thread_ts}"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Possibly an advanced configuration option for this class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree that this could be a nice config option.

Just thinking out:

  • If the inbound message was already in a thread, it seems Always Right to respond in-thread.
  • If the inbound message was not already in a thread, maybe that's when some kind of always_thread_response option would dictate?

Copy link
Copy Markdown
Contributor Author

@GitOnUp GitOnUp Aug 21, 2023

Choose a reason for hiding this comment

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

Oh, that's not even what I was meaning to discuss here but that's a good point and I will chase that.

The thing I was getting at here is that build_default_context IIRC will be used for recalling previous messages from a file using that ID, and I wanted to consider whether, if you're talking to the agent from within a thread, the requests to the bot from the outer channel should be considered (ETA: and from within other threads for that matter)

Copy link
Copy Markdown
Contributor

@eob eob left a comment

Choose a reason for hiding this comment

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

This looks awesome! A few small requests w/r/t where you can stash some constants.

Excited to start using this in our own bots :)

Comment thread src/steamship/agents/mixins/transports/slack.py Outdated
Comment thread src/steamship/agents/mixins/transports/slack.py Outdated
thread_ts = get_block_thread_ts(incoming_message)
# TODO (george) not sure this is always what we want, unfortunately. Some bots may want to keep a
# conversation going through threads, others may not?
context_id = chat_id if not thread_ts else f"{chat_id}-{thread_ts}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree that this could be a nice config option.

Just thinking out:

  • If the inbound message was already in a thread, it seems Always Right to respond in-thread.
  • If the inbound message was not already in a thread, maybe that's when some kind of always_thread_response option would dictate?

@GitOnUp GitOnUp requested a review from eob August 22, 2023 00:19
@GitOnUp
Copy link
Copy Markdown
Contributor Author

GitOnUp commented Aug 22, 2023

Still going to give this a bit of ad hoc testing before merge

eob
eob previously approved these changes Aug 22, 2023
Copy link
Copy Markdown
Contributor

@eob eob left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

@GitOnUp GitOnUp requested a review from eob August 23, 2023 02:22
@GitOnUp
Copy link
Copy Markdown
Contributor Author

GitOnUp commented Aug 23, 2023

Sorry for churn, found a couple of issues (didn't realize config values stripped Enum-ness so comparisons failed)

E: but now it doesn't look like it in tests? Blerg

@GitOnUp GitOnUp marked this pull request as draft August 23, 2023 16:22
@GitOnUp GitOnUp removed the request for review from eob August 23, 2023 16:22
@GitOnUp GitOnUp requested a review from eob August 23, 2023 17:30
@GitOnUp GitOnUp marked this pull request as ready for review August 23, 2023 17:30
@GitOnUp GitOnUp merged commit ba6a3ec into main Aug 24, 2023
@douglas-reid douglas-reid deleted the slack-thread-support branch September 28, 2023 02:20
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