Enhance SlackWebMessagingIntegration to support reply_broadcast optio… #2054
Conversation
…n in message sending
|
👋 @MikaKerman |
WalkthroughThe PR adds a new optional Changes
Sequence DiagramsequenceDiagram
participant Client
participant SlackWebMessagingIntegration
participant _send_message
participant SlackAPI as Slack API
Client->>SlackWebMessagingIntegration: __init__(reply_broadcast=True)
SlackWebMessagingIntegration->>SlackWebMessagingIntegration: self.reply_broadcast = True
Client->>SlackWebMessagingIntegration: reply_to_message(destination, message, thread_ts)
SlackWebMessagingIntegration->>_send_message: _send_message(..., reply_broadcast=self.reply_broadcast)
_send_message->>SlackAPI: chat_postMessage(..., reply_broadcast=True)
SlackAPI-->>_send_message: MessageSendResult
_send_message-->>SlackWebMessagingIntegration: Result
SlackWebMessagingIntegration-->>Client: Result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
94-100: Retry path dropsreply_broadcast, leading to inconsistent behavior on resendThe new
reply_broadcastparameter is threaded into_send_messageand down tochat_postMessage, but the retry path does not pass it:return self._send_message(destination, formatted_message, thread_ts)This means any retried send (once
_handle_send_erris adjusted to allow retries for recoverable errors) will silently revert toreply_broadcast=False, which is inconsistent with the initial call and could surprise callers.You can preserve behavior by forwarding the flag:
def _send_message( self, destination: Channel, formatted_message: FormattedBlockKitMessage, thread_ts: Optional[str] = None, reply_broadcast: bool = False, ) -> MessageSendResult[SlackWebMessageContext]: @@ except SlackApiError as e: self._handle_send_err(e, destination) - return self._send_message(destination, formatted_message, thread_ts) + return self._send_message( + destination, + formatted_message, + thread_ts=thread_ts, + reply_broadcast=reply_broadcast, + )Even though
_handle_send_errcurrently always raises, fixing this now keeps the retry logic correct and future-proof if error handling is relaxed later.Also applies to: 107-111
🧹 Nitpick comments (2)
elementary/messages/messaging_integrations/slack_web.py (2)
44-53: Constructor wiring forreply_broadcastlooks good; consider exposing it viafrom_tokenas wellThe new
reply_broadcastflag is correctly added and stored on the instance. However,from_tokencannot configure it and always uses the defaultFalse, which makes the feature harder to use via the primary factory constructor.I suggest extending
from_tokento accept and pass throughreply_broadcastso callers don’t need to bypass it just to enable broadcast replies:- @classmethod - def from_token( - cls, token: str, tracking: Optional[Tracking] = None - ) -> "SlackWebMessagingIntegration": - client = WebClient(token=token) - client.retry_handlers.append(RateLimitErrorRetryHandler(max_retry_count=5)) - return cls(client, tracking) + @classmethod + def from_token( + cls, + token: str, + tracking: Optional[Tracking] = None, + reply_broadcast: bool = False, + ) -> "SlackWebMessagingIntegration": + client = WebClient(token=token) + client.retry_handlers.append(RateLimitErrorRetryHandler(max_retry_count=5)) + return cls(client, tracking, reply_broadcast=reply_broadcast)
86-90: Reply wiring correctly propagatesreply_broadcast; global flag vs per-message override is a design choiceUsing
reply_broadcast=self.reply_broadcastwhen replying to a message correctly ensures that only threaded replies are affected by this flag and plainsend_messagecalls remain unchanged.This does make
reply_broadcastan integration-wide setting rather than something that can be toggled per reply. If you foresee needing per-message control (e.g., some replies broadcast, some not, within the same integration instance), consider allowingreply_to_messageto accept an optionalreply_broadcastoverride in the future. For now, the implementation is consistent and safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
elementary/messages/messaging_integrations/slack_web.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
elementary/messages/messaging_integrations/slack_web.py (2)
elementary/messages/messaging_integrations/base_messaging_integration.py (1)
MessageSendResult(19-22)elementary/clients/slack/slack_message_builder.py (2)
blocks(33-34)attachments(37-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: code-quality
- GitHub Check: test / test
MikaKerman
deleted the
app-315-broadcast-slack-resolution-notifications-in-channel
branch
…n in message sending
Summary by CodeRabbit