[INTPROD-9643] Add thread support for reaction message handlers #114
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds support for handling reactions on thread replies by capturing the item_user field and updating the processing logic to branch on thread messages vs. regular messages.
- Extended
Reactionpayload withitem_userand added a corresponding property. - Updated
_process_reaction_message_handlersto ignore non-bot thread replies and to defer to_is_message_from_botfor top-level messages. - Refined
_is_message_from_botto separate missing-message and non-bot message logging and return behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| omnibot/services/slack/reaction.py | Added item_user into payload with property and adjusted logs. |
| omnibot/processor.py | Branched on item_user, refined ignore logic, and improved logging in _is_message_from_bot. |
Comments suppressed due to low confidence (3)
omnibot/services/slack/reaction.py:24
- Add consistent punctuation (e.g., a trailing period) to the log message to match existing style guidelines.
logger.error("Reaction event is missing reaction attribute")
omnibot/services/slack/reaction.py:22
- Catching all exceptions is too broad; consider catching specific exceptions (e.g., KeyError) to avoid masking unrelated errors.
except Exception:
omnibot/services/slack/reaction.py:38
- Add tests to cover cases where
item_useris present and absent to ensure thread reaction handling behaves as expected.
self._payload["item_user"] = event.get("item_user")
Co-authored-by: Copilot <[email protected]>
| if not _is_message_from_bot(bot, item_channel, item_ts): | ||
| if item_user is not None: | ||
| if item_user != bot.user_id: | ||
| statsd.incr("event.ignored") |
There was a problem hiding this comment.
do we get called with irrelevant events?
There was a problem hiding this comment.
Yes, an event is sent for every bot that is in the channel.
| handler_called = False | ||
| item_channel = reaction.item_channel | ||
| item_ts = reaction.item_ts | ||
| item_user = reaction.item_user |
There was a problem hiding this comment.
Are we differentiating reaction user too?
There was a problem hiding this comment.
Yes, the user who made the reaction is set in BaseMessage using user field
There was a problem hiding this comment.
Pull Request Overview
This PR adds thread support for reaction message handlers by updating reaction payload extraction and refining message processing.
- Captures the item_user field in reaction events, and exposes it via a new property in reaction.py.
- Updates the reaction message handler in processor.py to use item_user when present, with a fallback to the bot message check, and refines logging in _is_message_from_bot.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| omnibot/services/slack/reaction.py | Adds extraction of the item_user field and a corresponding property to expose it. |
| omnibot/processor.py | Updates reaction handler logic to prioritize item_user and refines log level handling. |
Adds support for reactions made on thread replies.
The
item_userfield is only present for reaction events on messages such as those made withchat.postMessage. Responses made through slack command responses look similar to messages but reactions events on them do not containitem_userfield and require a fallback check to determine which bot sent the response.