sending to channel-id, channel resolving and listing#2165
sending to channel-id, channel resolving and listing#2165ofek1weiss wants to merge 7 commits intomasterfrom
Conversation
|
👋 @ofek1weiss |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNormalize Slack channel inputs, add channel ID detection and caching, centralize paginated conversations listing, and implement ChangesSlack channel resolution & listing
Sequence DiagramsequenceDiagram
participant Client as Client
participant Integr as SlackWebIntegration
participant Cache as ChannelCache
participant Slack as Slack API
Client->>Integr: send message / request resolve_channel(name)
Integr->>Cache: lookup(normalized_input, only_public)
alt cached
Cache-->>Integr: ResolvedChannel(id,name)
Integr-->>Client: proceed with channel id
else not cached
alt input looks like ID
Integr->>Slack: conversations_info(id)
Slack-->>Integr: channel info
else name
Integr->>Slack: conversations_list(cursor...) (paged)
Slack-->>Integr: page of channels / next_cursor
Integr->>Slack: ...repeat until found or exhausted
end
Integr->>Cache: store ResolvedChannel
Integr-->>Client: return ResolvedChannel / id
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (2)
elementary/messages/messaging_integrations/slack_web.py (2)
83-83: Consider using modern type hint syntax for consistency.The dataclasses use Python 3.10+ syntax (
list[...],int | None), but the cache uses legacyDict/Tuple. Consider aligning the style:- self._channel_cache: Dict[Tuple[str, bool], ResolvedChannel] = {} + self._channel_cache: dict[tuple[str, bool], ResolvedChannel] = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/messaging_integrations/slack_web.py` at line 83, Update the cache annotation to use modern PEP 585 syntax to match the project style: replace the legacy typing usage on the field self._channel_cache: Dict[Tuple[str, bool], ResolvedChannel] = {} with the builtin generic form (e.g., dict[tuple[str, bool], ResolvedChannel]) and remove any now-unnecessary typing imports (Dict, Tuple) or keep them only if used elsewhere; ensure the class attribute ResolvedChannel reference remains unchanged and that the file's minimum Python version supports PEP 585.
241-245: Consider addingonly_publicparameter for API consistency.For consistency with
resolve_channel,get_channelscould accept anonly_publicparameter to filter the channel types fetched. This would allow callers to list only public channels when needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/messaging_integrations/slack_web.py` around lines 241 - 245, Add an optional only_public: bool = False parameter to the get_channels method signature (alongside cursor and timeout_seconds) and thread it through the internal logic that builds the Slack API request so only public channels are returned when True (mirror the filtering behavior used by resolve_channel). Update any calls to get_channels and related unit tests to pass the new parameter where needed, and ensure ChannelsResponse semantics remain unchanged when only_public is False.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Around line 176-187: The channel listing ignores the only_public flag; modify
_list_conversations to accept an only_public: bool parameter and set types to
"public_channel" when only_public is True and "public_channel,private_channel"
otherwise, then update callers _iter_channels and resolve_channel to pass the
only_public argument through (ensure _iter_channels forwards only_public to
_list_conversations and resolve_channel passes the only_public it receives when
invoking _iter_channels) so private channels are excluded when only_public=True.
- Around line 268-274: When handling SlackApiError in the block that checks
err.response.status_code == HTTPStatus.TOO_MANY_REQUESTS, defensively read the
Retry-After header using err.response.headers.get("Retry-After") instead of
indexing, and only convert to int and assign to channels_response.retry_after if
the header is present and a valid integer; otherwise set
channels_response.retry_after to a sensible default (e.g., 1 or 0) or leave it
unset. Update the SlackApiError handler surrounding SlackApiError,
HTTPStatus.TOO_MANY_REQUESTS, err.response.headers, and
channels_response.retry_after to avoid KeyError/ValueError from a missing or
malformed header.
---
Nitpick comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Line 83: Update the cache annotation to use modern PEP 585 syntax to match the
project style: replace the legacy typing usage on the field self._channel_cache:
Dict[Tuple[str, bool], ResolvedChannel] = {} with the builtin generic form
(e.g., dict[tuple[str, bool], ResolvedChannel]) and remove any now-unnecessary
typing imports (Dict, Tuple) or keep them only if used elsewhere; ensure the
class attribute ResolvedChannel reference remains unchanged and that the file's
minimum Python version supports PEP 585.
- Around line 241-245: Add an optional only_public: bool = False parameter to
the get_channels method signature (alongside cursor and timeout_seconds) and
thread it through the internal logic that builds the Slack API request so only
public channels are returned when True (mirror the filtering behavior used by
resolve_channel). Update any calls to get_channels and related unit tests to
pass the new parameter where needed, and ensure ChannelsResponse semantics
remain unchanged when only_public is False.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b36fe43-522b-4a03-b091-5f2ac4000f29
📒 Files selected for processing (1)
elementary/messages/messaging_integrations/slack_web.py
…dd-slack-channel-resolution
There was a problem hiding this comment.
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)
161-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn after a successful join so the resend path can run.
After
_join_channel()succeeds, execution still falls through to the unconditionalMessagingIntegrationErrorat Line 172. That means_send_message()never reaches its retry for thenot_in_channelrecovery path.Proposed fix
if err_type == "not_in_channel": logger.info( f'Elementary app is not in the channel "{channel_name}". Attempting to join.' ) channel_id = self.resolve_channel(channel_name, only_public=True).id self._join_channel(channel_id=channel_id) logger.info(f"Joined channel {channel_name}") + 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(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/messaging_integrations/slack_web.py` around lines 161 - 173, The failure path for err_type "not_in_channel" falls through to the unconditional MessagingIntegrationError because we never return after successfully joining; update the branch in _send_message (the block that calls resolve_channel and _join_channel) to return immediately after logger.info(f"Joined channel {channel_name}") so the outer resend/retry path can run (or alternatively trigger the resend logic there). Ensure the change references the existing resolve_channel(...) call and the subsequent _join_channel(channel_id=...) call so the method exits after a successful join.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Around line 161-173: The failure path for err_type "not_in_channel" falls
through to the unconditional MessagingIntegrationError because we never return
after successfully joining; update the branch in _send_message (the block that
calls resolve_channel and _join_channel) to return immediately after
logger.info(f"Joined channel {channel_name}") so the outer resend/retry path can
run (or alternatively trigger the resend logic there). Ensure the change
references the existing resolve_channel(...) call and the subsequent
_join_channel(channel_id=...) call so the method exits after a successful join.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e71ca19-1c5a-483b-8a5f-3c2de868524c
📒 Files selected for processing (1)
elementary/messages/messaging_integrations/slack_web.py
There was a problem hiding this comment.
Actionable comments posted: 1
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)
161-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn after the successful join path.
After Lines 165-167,
_handle_send_err()still falls through to the unconditionalMessagingIntegrationErrorat Lines 172-174. That makes the retry in_send_message()unreachable fornot_in_channel, so joinable public channels still fail on the first send.Suggested fix
if err_type == "not_in_channel": logger.info( f'Elementary app is not in the channel "{channel_name}". Attempting to join.' ) channel_id = self.resolve_channel(channel_name, only_public=True).id self._join_channel(channel_id=channel_id) logger.info(f"Joined channel {channel_name}") + 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." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/messaging_integrations/slack_web.py` around lines 161 - 173, The code path in _handle_send_err that handles err_type == "not_in_channel" calls resolve_channel(...) and _join_channel(...) but then falls through to raise MessagingIntegrationError, preventing _send_message's retry; change the not_in_channel branch in _handle_send_err so that after successfully resolving the channel and calling self._join_channel(channel_id=...), the method returns (e.g., return None) instead of continuing to the final raise, allowing _send_message to retry; keep the existing logger.info calls and only raise MessagingIntegrationError in the other branches (e.g., channel_not_found and the final unconditional failure).
♻️ Duplicate comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
176-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
only_publicis inverted here.Line 179 currently includes private channels when
only_public=Trueand excludes them whenFalse. That flipsresolve_channel()'s contract and makesget_channels()list only public channels by default.Suggested fix
- types = "public_channel,private_channel" if only_public else "public_channel" + types = "public_channel" if only_public else "public_channel,private_channel"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/messaging_integrations/slack_web.py` around lines 176 - 183, The boolean logic for channel type selection in _list_conversations is inverted: change the ternary that sets types so that when only_public is True you use "public_channel" and when False you use "public_channel,private_channel"; update the types assignment in _list_conversations (and verify callers like resolve_channel and get_channels rely on the corrected behavior) so only_public actually restricts results to public channels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Around line 186-190: The code is currently assigning the entire
response_metadata dict to next_cursor which causes callers like _iter_channels()
and get_channels() to receive a dict instead of Slack's next-cursor string;
change the assignment so it extracts the string value: replace the current
next_cursor assignment with code that does response.get("response_metadata",
{}).get("next_cursor") (or equivalent safe extraction) so next_cursor is the
actual cursor string (referencing the variables next_cursor and response and the
functions _iter_channels / get_channels to locate usage).
---
Outside diff comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Around line 161-173: The code path in _handle_send_err that handles err_type
== "not_in_channel" calls resolve_channel(...) and _join_channel(...) but then
falls through to raise MessagingIntegrationError, preventing _send_message's
retry; change the not_in_channel branch in _handle_send_err so that after
successfully resolving the channel and calling
self._join_channel(channel_id=...), the method returns (e.g., return None)
instead of continuing to the final raise, allowing _send_message to retry; keep
the existing logger.info calls and only raise MessagingIntegrationError in the
other branches (e.g., channel_not_found and the final unconditional failure).
---
Duplicate comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Around line 176-183: The boolean logic for channel type selection in
_list_conversations is inverted: change the ternary that sets types so that when
only_public is True you use "public_channel" and when False you use
"public_channel,private_channel"; update the types assignment in
_list_conversations (and verify callers like resolve_channel and get_channels
rely on the corrected behavior) so only_public actually restricts results to
public channels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a68a8bda-068b-4c69-9baf-a6ade192880d
📒 Files selected for processing (1)
elementary/messages/messaging_integrations/slack_web.py
| channels = response.get("channels", cast(List[dict], [])) | ||
| next_cursor = response.get( | ||
| "response_metadata", cast(dict, {}).get("next_cursor") | ||
| ) | ||
| return channels, next_cursor |
There was a problem hiding this comment.
Extract next_cursor, not the whole metadata dict.
Lines 187-189 assign response_metadata itself to next_cursor. On the second page, _iter_channels() and get_channels() will pass a dict as the cursor argument instead of Slack's next-cursor string.
Suggested fix
- next_cursor = response.get(
- "response_metadata", cast(dict, {}).get("next_cursor")
- )
+ next_cursor = response.get("response_metadata", {}).get("next_cursor")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@elementary/messages/messaging_integrations/slack_web.py` around lines 186 -
190, The code is currently assigning the entire response_metadata dict to
next_cursor which causes callers like _iter_channels() and get_channels() to
receive a dict instead of Slack's next-cursor string; change the assignment so
it extracts the string value: replace the current next_cursor assignment with
code that does response.get("response_metadata", {}).get("next_cursor") (or
equivalent safe extraction) so next_cursor is the actual cursor string
(referencing the variables next_cursor and response and the functions
_iter_channels / get_channels to locate usage).
There was a problem hiding this comment.
+1 — this is still present in the final code. response.get("response_metadata", cast(dict, {}).get("next_cursor")) evaluates the cast(...) part eagerly to None, so the whole thing is just response.get("response_metadata", None) — returns the entire dict, not the cursor string.
There was a problem hiding this comment.
@MikaKerman — correct, and the root cause is subtle: cast() is a pure type-hint helper with no runtime effect, so cast(dict, {}).get("next_cursor") is literally just {}.get("next_cursor") → None. Python evaluates default arguments eagerly, so the entire expression becomes response.get("response_metadata", None), returning whatever "response_metadata" maps to in the response — the full metadata dict — instead of the cursor string.
The fix is to chain .get() calls so the fallback is an empty dict consumed by the second call:
next_cursor = response.get("response_metadata", {}).get("next_cursor")Without this fix, every call after the first page passes a dict as cursor to conversations_list, which will either error out or silently restart from the beginning, causing an infinite loop or incorrect pagination in both _iter_channels and get_channels.
| 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.resolve_channel(channel_name, only_public=True).id |
There was a problem hiding this comment.
Pre-existing, but since you're touching this method anyway — after the join succeeds, execution falls through to the unconditional raise MessagingIntegrationError(...) at the bottom. That means _send_message's retry path (return self._send_message(...) after the except) is unreachable for the not_in_channel case. Adding a return after logger.info(f"Joined channel {channel_name}") would let the retry actually work.
| self._channel_cache[cache_key] = resolved | ||
| return resolved | ||
|
|
||
| def get_channels( |
There was a problem hiding this comment.
Nit: resolve_channel accepts only_public, but get_channels doesn't and always passes the default to _list_conversations. Might be worth threading the parameter through for consistency, especially if callers ever need to list only public channels.
| if isinstance(retry_after, str) and retry_after.isdigit(): | ||
| channels_response.retry_after = int(retry_after) | ||
| else: | ||
| channels_response.retry_after = 60 |
There was a problem hiding this comment.
Nit: the hardcoded 60 feels arbitrary — the method itself has a timeout_seconds of 15 by default, so telling callers to retry after 60s is a pretty big jump. Might be worth making this configurable or at least documenting the reasoning.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
188-190:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
next_cursorextraction returns the entire metadata dict instead of the cursor string.
cast(dict, {}).get("next_cursor")is evaluated eagerly toNone, so this becomesresponse.get("response_metadata", None)— returning the whole metadata dict. On subsequent pages,_iter_channelsandget_channelspass a dict ascursor, which will break pagination (infinite loop or error).🐛 Proposed fix
- next_cursor = response.get( - "response_metadata", cast(dict, {}).get("next_cursor") - ) + next_cursor = response.get("response_metadata", {}).get("next_cursor")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elementary/messages/messaging_integrations/slack_web.py` around lines 188 - 190, The code that sets next_cursor is returning the entire response_metadata dict instead of the cursor string; fix the extraction in the pagination code (used by _iter_channels and get_channels) so you first fetch the metadata dict then call .get("next_cursor") on it (e.g., use response.get("response_metadata", {}) and then .get("next_cursor")), ensuring next_cursor is the string cursor (or None) rather than the full dict to avoid breaking pagination.
🧹 Nitpick comments (1)
elementary/messages/messaging_integrations/slack_web.py (1)
275-284: 💤 Low valueConsider using
try/exceptforRetry-Afterparsing.The current
isinstance(retry_after, str) and retry_after.isdigit()check will silently fall through to5if the Slack SDK returns the header as anintdirectly (edge case). Atry/exceptapproach handles all cases cleanly.♻️ Proposed refactor
except SlackApiError as err: if err.response.status_code == HTTPStatus.TOO_MANY_REQUESTS: retry_after = err.response.headers.get("Retry-After") - if isinstance(retry_after, str) and retry_after.isdigit(): - channels_response.retry_after = int(retry_after) - else: - # should never happen - as the header should always be valid. just a safety net. + try: + channels_response.retry_after = int(retry_after) + except (TypeError, ValueError): channels_response.retry_after = 5 break raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elementary/messages/messaging_integrations/slack_web.py` around lines 275 - 284, The retry-after parsing falls back to 5 if the header is not a digit-string, which misses cases where the SDK returns an integer or other numeric type; update the except block that catches SlackApiError in the function using SlackApiError and channels_response.retry_after to parse retry_after using a small try/except: obtain retry_after = err.response.headers.get("Retry-After"), then attempt to cast int(retry_after) inside try and assign to channels_response.retry_after, and on ValueError/TypeError set channels_response.retry_after = 5; keep the HTTPStatus.TOO_MANY_REQUESTS branch and the final raise unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Line 180: The ternary setting the conversation types is inverted: change the
assignment of the variable `types` so that when the `only_public` parameter is
True it sets `types` to "public_channel" and when False it sets `types` to
"public_channel,private_channel"; update the `types` assignment (the `types`
variable near the usage that filters Slack conversations) accordingly so
`only_public` behaves as intended.
---
Duplicate comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Around line 188-190: The code that sets next_cursor is returning the entire
response_metadata dict instead of the cursor string; fix the extraction in the
pagination code (used by _iter_channels and get_channels) so you first fetch the
metadata dict then call .get("next_cursor") on it (e.g., use
response.get("response_metadata", {}) and then .get("next_cursor")), ensuring
next_cursor is the string cursor (or None) rather than the full dict to avoid
breaking pagination.
---
Nitpick comments:
In `@elementary/messages/messaging_integrations/slack_web.py`:
- Around line 275-284: The retry-after parsing falls back to 5 if the header is
not a digit-string, which misses cases where the SDK returns an integer or other
numeric type; update the except block that catches SlackApiError in the function
using SlackApiError and channels_response.retry_after to parse retry_after using
a small try/except: obtain retry_after =
err.response.headers.get("Retry-After"), then attempt to cast int(retry_after)
inside try and assign to channels_response.retry_after, and on
ValueError/TypeError set channels_response.retry_after = 5; keep the
HTTPStatus.TOO_MANY_REQUESTS branch and the final raise unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 092572c6-1695-4345-b609-4f132d5e7985
📒 Files selected for processing (1)
elementary/messages/messaging_integrations/slack_web.py
2 participants
null
Summary by CodeRabbit
New Features
Refactor