Slack join channel recursion fix #1984
Conversation
MikaKerman
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
WalkthroughRestricts Slack channel lookups to optionally only public channels by adding an only_public flag to channel iteration and lookup; adjusts join-on-missing-channel flow to resolve using public-only lookups; updates method signatures and conversation_list types and preserves pagination and error paths. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant SlackWeb as SlackWebMessagingIntegration
participant SlackAPI as Slack API
Caller->>SlackWeb: send_message(channel_name, ...)
SlackWeb->>SlackAPI: send (by channel_id or name)
SlackAPI-->>SlackWeb: error: not_in_channel
SlackWeb->>SlackWeb: _handle_send_err(...)
SlackWeb->>SlackWeb: _get_channel_id(channel_name, only_public=True)
SlackWeb->>SlackWeb: _iter_channels(..., only_public=True)
SlackWeb->>SlackAPI: conversations_list(types="public_channel", cursor=...)
SlackAPI-->>SlackWeb: channels page
alt channel found
SlackWeb->>SlackAPI: conversations_join / retry send
SlackAPI-->>SlackWeb: ack
SlackWeb-->>Caller: success
else not found
SlackWeb-->>Caller: raise MessagingIntegrationError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
👋 @MikaKerman |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
110-127: Joining path can never retry: _handle_send_err always raises, preventing resendAs implemented, _handle_send_err ends with an unconditional MessagingIntegrationError, so _send_message’s retry path (return self._send_message(...)) is never reached—even after successfully joining a channel. This breaks the intended recover-on-join flow.
Suggested fix (outside changed hunk): only return after handling the recoverable case; raise for unrecoverable cases:
def _handle_send_err(self, err: SlackApiError, channel_name: str): if self.tracking: self.tracking.record_internal_exception(err) err_type = err.response.data.get("error") if err_type == "not_in_channel": logger.info(f'Elementary app is not in the channel "{channel_name}". Attempting to join.') channel_id = self._get_channel_id(channel_name) self._join_channel(channel_id=channel_id) logger.info(f"Joined channel {channel_name}") return # allow caller to retry send elif err_type == "channel_not_found": raise MessagingIntegrationError( f"Channel {channel_name} was not found by the Elementary app. Please add the app to the channel." ) # For all other errors, propagate a failure (no retry) raise MessagingIntegrationError(f"Failed to send a message to channel - {channel_name}: {err_type}")
🧹 Nitpick comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
158-161: Optional: guard against duplicate loads in concurrent callsIf multiple threads/processes call _ensure_channels_loaded concurrently, you may end up running _load_channels multiple times. Not harmful, but avoidable with a simple lock.
Outside this hunk, consider:
# at top of file import threading # in __init__ self._channels_cache_lock = threading.Lock() # here def _ensure_channels_loaded(self) -> None: if self._channels_cache is None: with self._channels_cache_lock: if self._channels_cache is None: self._load_channels()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- 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(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
elementary/messages/messaging_integrations/slack_web.py (1)
elementary/messages/messaging_integrations/exceptions.py (1)
MessagingIntegrationError(1-2)
🔇 Additional comments (2)
elementary/messages/messaging_integrations/slack_web.py (2)
47-47: Good addition: lazy channels cache fieldInitializing the cache as Optional[Dict[str, str]] with None for lazy loading is appropriate.
162-164: Public refresh hook is goodProviding refresh_channels_cache is useful for explicit invalidation when channels are added/renamed.
| def _load_channels(self) -> None: | ||
| self._channels_cache = {} | ||
| try: | ||
| for channel in self._iter_channels(): | ||
| self._channels_cache[channel["name"]] = channel["id"] | ||
| logger.debug(f"Loaded {len(self._channels_cache)} channels into cache") | ||
| except Exception as e: | ||
| logger.error(f"Failed to load channels: {e}") | ||
| # Keep cache as empty dict rather than None to avoid retrying constantly | ||
| self._channels_cache = {} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid exposing a partially-built cache; build locally then swap; improve error handling and key normalization
As written, another thread can observe a partially populated dict because you assign self._channels_cache = {} up front and then mutate in-place. Build into a local dict and atomically swap at the end. Also, prefer name_normalized where available, normalize keys to lowercase to make lookups resilient, and include stack traces in logs. Narrowing exceptions helps too.
Apply this diff within the method:
def _load_channels(self) -> None:
- self._channels_cache = {}
- try:
- for channel in self._iter_channels():
- self._channels_cache[channel["name"]] = channel["id"]
- logger.debug(f"Loaded {len(self._channels_cache)} channels into cache")
- except Exception as e:
- logger.error(f"Failed to load channels: {e}")
- # Keep cache as empty dict rather than None to avoid retrying constantly
- self._channels_cache = {}
+ try:
+ channels_by_name: Dict[str, str] = {}
+ for channel in self._iter_channels():
+ # Prefer normalized names; fall back to name. Store lowercase keys.
+ name = channel.get("name_normalized") or channel.get("name")
+ chan_id = channel.get("id")
+ if not name or not chan_id:
+ continue
+ channels_by_name[name.lower()] = chan_id
+ self._channels_cache = channels_by_name
+ logger.debug(f"Loaded {len(channels_by_name)} channels into cache")
+ except SlackApiError as e:
+ logger.exception(f"Failed to load channels via Slack API: {e}")
+ # Keep cache as empty dict rather than None to avoid retrying constantly
+ self._channels_cache = {}
+ except Exception:
+ logger.exception("Unexpected error while loading channels")
+ self._channels_cache = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _load_channels(self) -> None: | |
| self._channels_cache = {} | |
| try: | |
| for channel in self._iter_channels(): | |
| self._channels_cache[channel["name"]] = channel["id"] | |
| logger.debug(f"Loaded {len(self._channels_cache)} channels into cache") | |
| except Exception as e: | |
| logger.error(f"Failed to load channels: {e}") | |
| # Keep cache as empty dict rather than None to avoid retrying constantly | |
| self._channels_cache = {} | |
| def _load_channels(self) -> None: | |
| try: | |
| channels_by_name: Dict[str, str] = {} | |
| for channel in self._iter_channels(): | |
| # Prefer normalized names; fall back to name. Store lowercase keys. | |
| name = channel.get("name_normalized") or channel.get("name") | |
| chan_id = channel.get("id") | |
| if not name or not chan_id: | |
| continue | |
| channels_by_name[name.lower()] = chan_id | |
| self._channels_cache = channels_by_name | |
| logger.debug(f"Loaded {len(channels_by_name)} channels into cache") | |
| except SlackApiError as e: | |
| logger.exception(f"Failed to load channels via Slack API: {e}") | |
| # Keep cache as empty dict rather than None to avoid retrying constantly | |
| self._channels_cache = {} | |
| except Exception: | |
| logger.exception("Unexpected error while loading channels") | |
| self._channels_cache = {} |
| self._ensure_channels_loaded() | ||
|
|
||
| if channel_name not in self._channels_cache: | ||
| raise MessagingIntegrationError(f"Channel {channel_name} not found") | ||
|
|
||
| return self._channels_cache[channel_name] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize inputs, short-circuit IDs, and one-shot refresh on miss
To preserve previous behavior on newly created/renamed channels and to accept IDs directly, normalize lookups and refresh once on miss before failing. Also, assert the cache is initialized for type-checkers.
Apply this diff:
def _get_channel_id(self, channel_name: str) -> str:
- self._ensure_channels_loaded()
-
- if channel_name not in self._channels_cache:
- raise MessagingIntegrationError(f"Channel {channel_name} not found")
-
- return self._channels_cache[channel_name]
+ self._ensure_channels_loaded()
+ assert self._channels_cache is not None
+ # Fast-path: if an ID is provided, return it as-is.
+ if channel_name and channel_name[0] in ("C", "G") and channel_name[1:].isalnum():
+ return channel_name
+ key = channel_name.lstrip("#").lower()
+ channel_id = self._channels_cache.get(key)
+ if channel_id is None:
+ # One-shot refresh to mitigate stale cache before failing.
+ self.refresh_channels_cache()
+ assert self._channels_cache is not None
+ channel_id = self._channels_cache.get(key)
+ if channel_id is None:
+ raise MessagingIntegrationError(f"Channel {channel_name} not found")
+ return channel_id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._ensure_channels_loaded() | |
| if channel_name not in self._channels_cache: | |
| raise MessagingIntegrationError(f"Channel {channel_name} not found") | |
| return self._channels_cache[channel_name] | |
| def _get_channel_id(self, channel_name: str) -> str: | |
| self._ensure_channels_loaded() | |
| assert self._channels_cache is not None | |
| # Fast-path: if an ID is provided, return it as-is. | |
| if channel_name and channel_name[0] in ("C", "G") and channel_name[1:].isalnum(): | |
| return channel_name | |
| key = channel_name.lstrip("#").lower() | |
| channel_id = self._channels_cache.get(key) | |
| if channel_id is None: | |
| # One-shot refresh to mitigate stale cache before failing. | |
| self.refresh_channels_cache() | |
| assert self._channels_cache is not None | |
| channel_id = self._channels_cache.get(key) | |
| if channel_id is None: | |
| raise MessagingIntegrationError(f"Channel {channel_name} not found") | |
| return channel_id |
🤖 Prompt for AI Agents
In elementary/messages/messaging_integrations/slack_web.py around lines 166 to
171, the channel lookup doesn't normalize input, doesn't short-circuit when
passed a channel ID, and never retries the cache on a miss; update the method to
normalize channel_name (strip and lower or otherwise canonicalize as used by the
cache), detect and immediately return when channel_name looks like an ID (e.g.,
starts with "C" or matches the ID pattern) to short-circuit, assert the cache is
initialized for type-checking, and if a normalized lookup misses then call
_ensure_channels_loaded() one more time to refresh the cache and retry before
raising MessagingIntegrationError. Ensure the final branch raises the error
including the original channel_name.
09b4418 to
2b4d5cd
Compare
MikaKerman
had a problem deploying
to
elementary_test_env
GitHub Actions
Error
MikaKerman
changed the title
| f'Elementary app is not in the channel "{channel_name}". Attempting to join.' | ||
| ) | ||
| channel_id = self._get_channel_id(channel_name) | ||
| channel_id = self._get_channel_id(channel_name, only_public=False) |
There was a problem hiding this comment.
Here it should be only_public=True no?
MikaKerman
force-pushed
the
fix/slack-channels-cache
branch
from
2b4d5cd to
eaf7e63
Compare
MikaKerman
had a problem deploying
to
elementary_test_env
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
109-127: Fix control flow: don’t raise after successful join; return to allow retryAfter handling "not_in_channel" and joining, the function still hits the unconditional raise below, preventing the retry in _send_message from ever executing. Early-return after a successful join and only raise for unknown errors.
Apply this diff:
if err_type == "not_in_channel": logger.info( f'Elementary app is not in the channel "{channel_name}". Attempting to join.' ) - channel_id = self._get_channel_id(channel_name, only_public=True) + channel_id = self._get_channel_id(channel_name, only_public=True) self._join_channel(channel_id=channel_id) logger.info(f"Joined channel {channel_name}") - elif err_type == "channel_not_found": + return + elif err_type == "channel_not_found": raise MessagingIntegrationError( f"Channel {channel_name} was not found by the Elementary app. Please add the app to the channel." ) - raise MessagingIntegrationError( - f"Failed to send a message to channel - {channel_name}" - ) + else: + raise MessagingIntegrationError( + f"Failed to send a message to channel - {channel_name}" + )
♻️ Duplicate comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
148-153: Handle channel IDs and normalized names; current logic can fail for IDs or “#name” inputsIf destination is already a channel ID (e.g., "C…" or "G…"), the current equality check against channel["name"] will never match, causing a false "not found" and breaking the not_in_channel join path. Also, comparisons should accept "#name" and use name_normalized for resilience (Slack lowercase normalization).
Apply this diff:
- def _get_channel_id(self, channel_name: str, only_public: bool = False) -> str: - for channel in self._iter_channels(only_public=only_public): - if channel["name"] == channel_name: - return channel["id"] - raise MessagingIntegrationError(f"Channel {channel_name} not found") + def _get_channel_id(self, channel_name: str, only_public: bool = False) -> str: + # Fast-path: if already a conversation ID, return it as-is. + if channel_name and channel_name[0] in ("C", "G") and channel_name[1:].isalnum(): + return channel_name + target = channel_name.lstrip("#").lower() + for channel in self._iter_channels(only_public=only_public): + name = channel.get("name_normalized") or channel.get("name") + if name and name.lower() == target: + return channel.get("id") # Slack guarantees 'id' here + raise MessagingIntegrationError(f"Channel {channel_name} not found")Run this to locate all call sites and ensure the updated signature (only_public) is accounted for where behavior matters:
#!/bin/bash # Find all usages of _get_channel_id across the repo with context. rg -nP --type=python -C3 '\b_get_channel_id\s*\(' # Additionally, check for places passing channel IDs directly into send paths # to validate that the short-circuit will cover them. rg -nP --type=python -C2 'chat_postMessage\([^)]*channel\s*=\s*([\'"]).*?\1'
🧹 Nitpick comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
146-146: Prefer keyword args in recursive call to avoid param order regressionsMinor readability/robustness improvement: pass cursor and only_public by name to make future signature changes safer and clearer.
- yield from self._iter_channels(next_cursor, only_public) + yield from self._iter_channels(cursor=next_cursor, only_public=only_public)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- 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 comments (2)
elementary/messages/messaging_integrations/slack_web.py (2)
117-117: Good: restrict lookup to public channels before joiningUsing only_public=True here ensures we only attempt to join public conversations, which is the only joinable type. This addresses the earlier feedback on scoping the search.
130-136: LGTM: types correctly constrained by only_publicPassing types="public_channel" when only_public is True and including private_channel otherwise matches Slack API expectations. Pagination, limits, and exclude_archived look good.
Added an option to only list public channels, since those are the ones we can join.
Summary by CodeRabbit