Fix: slack buttons #821
Fix: slack buttons #821
Conversation
📝 WalkthroughWalkthroughThe PR adds two Slack helper functions: one that removes action/button blocks from an original message and appends an italic acknowledgement, and one that extracts selection data from a block_actions payload and invokes the former. It modifies send_msg to reset text and clear update_msg_id when both buttons and feedback buttons are present so feedback is sent as a separate follow-up. In slack_interaction, it calls the new payload handler inside a guarded try/except and removes a previous fallback that derived files from nested message["message"]["files"]. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (2)
daras_ai_v2/slack_bot.py (1)
727-753: Consider documenting payload structure assumptions.The function directly accesses nested dictionary keys and list indices (lines 735-738, 743) without validation, which could raise
KeyError,IndexError, orBotIntegration.DoesNotExist. While the caller inrouters/slack_api.pywraps this in a try/except block, documenting the expected payload structure would improve maintainability.Consider adding a docstring that documents the expected payload structure:
def update_msg_ack_selection_from_payload(payload: dict): """ Convenience helper to process a Slack block_actions payload and update the original message by removing buttons and appending an italic confirmation. Performs BotIntegration lookup internally to get the token. + + Expected payload structure: + - payload["channel"]["id"] + - payload["team"]["id"] + - payload["container"]["message_ts"] + - payload["actions"][0] (non-empty list) + - payload["message"]["blocks"] (optional) + - payload["message"]["text"] (optional) + + Raises: + KeyError: If required payload keys are missing + IndexError: If actions list is empty + BotIntegration.DoesNotExist: If no matching integration found """routers/slack_api.py (1)
175-179: Consider more specific exception handling.While catching all exceptions ensures Slack interaction processing continues regardless of errors, the bare
Exceptioncatch is overly broad. Consider catching more specific exceptions or adding a log statement for debugging.Apply this diff to improve exception handling:
# Immediately update the original message to remove buttons and show selection try: update_msg_ack_selection_from_payload(data) - except Exception as e: + except (KeyError, IndexError, BotIntegration.DoesNotExist) as e: + print(f"Failed to update message acknowledgment: {e}") capture_exception(e)Alternatively, if you want to keep the broad catch but add logging:
# Immediately update the original message to remove buttons and show selection try: update_msg_ack_selection_from_payload(data) except Exception as e: + print(f"Failed to update message acknowledgment: {e}") capture_exception(e)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
daras_ai_v2/bots.py(1 hunks)daras_ai_v2/slack_bot.py(1 hunks)routers/slack_api.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
daras_ai_v2/slack_bot.py (1)
bots/models/bot_integration.py (1)
BotIntegration(163-626)
routers/slack_api.py (1)
daras_ai_v2/slack_bot.py (1)
update_msg_ack_selection_from_payload(727-753)
🪛 Ruff (0.14.1)
daras_ai_v2/slack_bot.py
714-714: Probable use of requests call without timeout
(S113)
routers/slack_api.py
178-178: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
daras_ai_v2/bots.py (1)
261-271: LGTM! Correctly prevents message update on feedback button flow.Setting
update_msg_id = Noneafter sending the initial message with buttons ensures that the subsequent message with feedback buttons is sent as a new message rather than updating the original. This prevents the button message from disappearing, which aligns with the PR objective.
nikochiko
force-pushed
the
slack-button
branch
from
9153877 to
e7c6517
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
daras_ai_v2/slack_bot.py (1)
714-724: Add timeout to the HTTP request.The
requests.postcall lacks a timeout parameter, which could cause the request to hang indefinitely if the Slack API becomes unresponsive.Apply this diff to add a timeout:
res = requests.post( "https://slack.com/api/chat.update", json={ "channel": channel, "ts": ts, "text": original_text or "", "blocks": new_blocks, }, headers={"Authorization": f"Bearer {token}"}, + timeout=30, )
🧹 Nitpick comments (2)
daras_ai_v2/slack_bot.py (2)
733-733: Remove redundant import.
BotIntegrationis already imported at the top of the file (Line 11), so this import inside the function is unnecessary.Apply this diff:
- from bots.models import BotIntegration - channel_id = payload["channel"]["id"]
738-739: Validate payload structure before accessing.The code accesses
payload["actions"][0]without verifying thatactionsexists or contains elements, which could raiseKeyErrororIndexErroron malformed payloads.Consider adding validation:
+ if not payload.get("actions"): + return action = payload["actions"][0] action_title = (action.get("text", {}) or {}).get("text") or ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/bots.py(1 hunks)daras_ai_v2/slack_bot.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- daras_ai_v2/bots.py
🧰 Additional context used
🧬 Code graph analysis (1)
daras_ai_v2/slack_bot.py (1)
bots/models/bot_integration.py (1)
BotIntegration(163-626)
🪛 Ruff (0.14.1)
daras_ai_v2/slack_bot.py
714-714: Probable use of requests call without timeout
(S113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (1)
daras_ai_v2/slack_bot.py (1)
743-751: I can see the function is called with exception handling in place. Let me verify the complete function implementation to confirm the context of the database lookup:Exception handling is already in place—no action required.
The function is called from
routers/slack_api.py:177within a try-except block that catches all exceptions and forwards them tocapture_exception(). TheDoesNotExistexception is a subclass of the baseObjectDoesNotExist, and a try-except forObjectDoesNotExistwill catchDoesNotExistexceptions for all models. Since the caller wraps the function invocation inexcept Exception, anyBotIntegration.DoesNotExistraised by the database lookup will be caught and logged appropriately.
Q/A checklist
How to check import time?
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.