sending to channel-id, channel resolving and listing#2165
Open
ofek1weiss wants to merge 1 commit intomasterfrom
Open
sending to channel-id, channel resolving and listing#2165ofek1weiss wants to merge 1 commit intomasterfrom
ofek1weiss wants to merge 1 commit intomasterfrom
Conversation
Contributor
|
👋 @ofek1weiss |
📝 WalkthroughWalkthroughRefactored Slack channel-lookup logic by replacing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SlackWeb as SlackWeb<br/>resolve_channel
participant Cache
participant SlackAPI as Slack API
Client->>SlackWeb: resolve_channel(channel)
SlackWeb->>SlackWeb: normalize_input(channel)
SlackWeb->>Cache: lookup (normalized, only_public)
alt Found in cache
Cache-->>SlackWeb: ResolvedChannel
else Not in cache
alt Channel looks like ID
SlackWeb->>SlackAPI: conversations_info(channel_id)
SlackAPI-->>SlackWeb: channel_info
else Channel is name
SlackWeb->>SlackAPI: conversations_list()
SlackAPI-->>SlackWeb: channels page
SlackWeb->>SlackWeb: iterate & match name
end
alt Channel found
SlackWeb->>Cache: store ResolvedChannel
Cache-->>SlackWeb: ✓
else Not found
SlackWeb->>SlackWeb: raise MessagingIntegrationError
end
end
SlackWeb-->>Client: ResolvedChannel
sequenceDiagram
participant Client
participant SlackWeb as SlackWeb<br/>get_channels
participant SlackAPI as Slack API
Client->>SlackWeb: get_channels(cursor, timeout_seconds)
loop Until cursor exhausted or timeout
SlackWeb->>SlackAPI: _list_conversations(cursor)
alt Rate limited (429)
SlackAPI-->>SlackWeb: HTTPStatus.TOO_MANY_REQUESTS
SlackWeb->>SlackWeb: extract retry_after header
SlackWeb-->>Client: ChannelsResponse(channels,<br/>retry_after, cursor)
Note over Client: Client should wait retry_after
else Success
SlackAPI-->>SlackWeb: channels + next_cursor
SlackWeb->>SlackWeb: accumulate ResolvedChannels
opt More pages & time remaining
SlackWeb->>SlackWeb: continue with next_cursor
end
end
end
SlackWeb-->>Client: ChannelsResponse(all_channels,<br/>None, None)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![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
Comment on lines
+176
to
+187
| def _list_conversations( | ||
| self, cursor: Optional[str] = None | ||
| ) -> Tuple[List[dict], Optional[str]]: | ||
| response = self.client.conversations_list( | ||
| cursor=cursor, | ||
| types="public_channel,private_channel", | ||
| exclude_archived=True, | ||
| limit=1000, | ||
| ) | ||
| channels = response.get("channels", []) | ||
| cursor = response.get("response_metadata", {}).get("next_cursor") | ||
| return channels, cursor |
There was a problem hiding this comment.
The only_public parameter is ignored throughout the channel iteration flow.
_list_conversations hardcodes types="public_channel,private_channel", making the only_public parameter in _iter_channels (line 194) and resolve_channel (line 212) dead code. When only_public=True is passed (e.g., line 165), private channels are still searched.
🐛 Proposed fix to respect `only_public`
def _list_conversations(
- self, cursor: Optional[str] = None
+ self, cursor: Optional[str] = None, only_public: bool = False
) -> Tuple[List[dict], Optional[str]]:
+ types = "public_channel" if only_public else "public_channel,private_channel"
response = self.client.conversations_list(
cursor=cursor,
- types="public_channel,private_channel",
+ types=types,
exclude_archived=True,
limit=1000,
)
- channels = response.get("channels", [])
- cursor = response.get("response_metadata", {}).get("next_cursor")
- return channels, cursor
+ channels = response.get("channels", [])
+ next_cursor = response.get("response_metadata", {}).get("next_cursor")
+ return channels, next_cursorThen update callers to pass the parameter through.
Also applies to: 191-207
🤖 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 -
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.
Comment on lines
+268
to
+274
| except SlackApiError as err: | ||
| if err.response.status_code == HTTPStatus.TOO_MANY_REQUESTS: | ||
| channels_response.retry_after = int( | ||
| err.response.headers["Retry-After"] | ||
| ) | ||
| break | ||
| raise |
There was a problem hiding this comment.
Add defensive check for Retry-After header.
While Slack's API typically includes this header with 429 responses, a missing header would cause a KeyError. Consider using .get() with a sensible default:
except SlackApiError as err:
if err.response.status_code == HTTPStatus.TOO_MANY_REQUESTS:
- channels_response.retry_after = int(
- err.response.headers["Retry-After"]
- )
+ retry_after = err.response.headers.get("Retry-After")
+ channels_response.retry_after = int(retry_after) if retry_after else 60
break
raise📝 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.
Suggested change
| except SlackApiError as err: | |
| if err.response.status_code == HTTPStatus.TOO_MANY_REQUESTS: | |
| channels_response.retry_after = int( | |
| err.response.headers["Retry-After"] | |
| ) | |
| break | |
| raise | |
| except SlackApiError as err: | |
| if err.response.status_code == HTTPStatus.TOO_MANY_REQUESTS: | |
| retry_after = err.response.headers.get("Retry-After") | |
| channels_response.retry_after = int(retry_after) if retry_after else 60 | |
| break | |
| raise |
🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
null
Summary by CodeRabbit
Release Notes
New Features
#prefix) and channel IDs.Refactor