Conversation
|
@gemini-cli /review |
|
🤖 Hi @ranlu, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
The pull request introduces support for Slack Socket Mode, which is a modern and recommended way to connect Slack bots without requiring public endpoints. The implementation correctly adds SocketModeClient support while maintaining backward compatibility with the legacy RTM mode.
🔍 General Feedback
- Great Documentation: The updates to the
README.mdand the inclusion ofslack_manifest.jsonmake it very easy for users to set up the new Socket Mode bot. - Clean Integration: The use of a shared
WebClientand routing Socket Mode events to existing handlers minimizes code duplication. - Reliability: The addition of logging and proper exception handling in the bot's main loop improves maintainability and debuggability.
| logger.info("Socket Mode client connected") | ||
| self.process_hello(self.socket_client, {}) | ||
| Event().wait() | ||
| else: |
There was a problem hiding this comment.
| else: | |
| self.socket_client.connect() | |
| logger.info("Socket Mode client connected") | |
| Event().wait() |
| logger.info("Initializing bot in Socket Mode") | ||
| self.socket_client = SocketModeClient( | ||
| app_token=self.app_token, | ||
| web_client=self.web_client, |
There was a problem hiding this comment.
| web_client=self.web_client, | |
| self.socket_client.socket_mode_request_listeners.append(self._handle_socket_mode_request) |
| botid = get_botid() | ||
| workerid = "seuron-worker-"+environ["DEPLOYMENT"] | ||
| broker_url = environ['AIRFLOW__CELERY__BROKER_URL'] | ||
| use_socket_mode = slack_app_token is not None |
There was a problem hiding this comment.
44f0583 to
060f3e3
Compare
|
@gemini-cli /review |
|
🤖 Hi @ranlu, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
This Pull Request introduces support for Slack's Socket Mode, which is the recommended connection method for modern Slack apps. It maintains backward compatibility with the legacy RTM Mode, allowing existing deployments to continue functioning.
🔍 General Feedback
- Well-structured integration: The implementation correctly refactors the
SeuronBotclass to handle bothSocketModeClientandRTMClientbased on the presence of an app-level token. - Improved Observability: The addition of logging and proper exception handling in the bot's main loop is a positive improvement over the previous
pass. - Comprehensive Documentation: The updates to
README.mdand the inclusion of a Slack app manifest make it easy for users to migrate to the new connection mode. - Robust Configuration: The setup script and cloud deployment configurations have been correctly updated to support the new token.
| for listener in self.hello_listeners: | ||
| listener() |
There was a problem hiding this comment.
🟡 It's better to use the logger for debugging information instead of print statements, as it allows for better control over log levels and formatting.
| for listener in self.hello_listeners: | |
| listener() | |
| def process_reaction(self, client, event: dict): | |
| logger.debug("reaction added") | |
| logger.debug(json.dumps(event, indent=4)) |
| self.socket_client.connect() | ||
| logger.info("Socket Mode client connected") | ||
| self.process_hello(self.socket_client, {}) | ||
| Event().wait() |
There was a problem hiding this comment.
🟢 Consider adding a brief comment here to clarify that this blocks the main thread to keep the Socket Mode connection alive while background tasks continue in their threads.
| Event().wait() | |
| # Block the main thread to keep the process alive | |
| Event().wait() |
Add support of socket mode slack bot, slack no longer allow user to create new RTM bots.