sending to channel-id, channel resolving and listing#2165
sending to channel-id, channel resolving and listing#2165ofek1weiss wants to merge 5 commits intomasterfrom
Conversation
|
👋 @ofek1weiss |
📝 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🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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.
2 participants
null
Summary by CodeRabbit
New Features
Refactor