Slackbot: per-message dedupe; ignore only edits while in-progress #1226
Conversation
…cate bot_auth line
…ignore only edits while in-progress\n\n- Parse edit events (event.subtype == message_changed) including nested message.ts/text\n- Use message_ts as idempotency key so replies/new mentions are unaffected\n- Post polite notice only on edit duplicates; skip quiet otherwise\n- Mark completion by message key
…edit and non-edit events
…(event.message or {}) usage per Copilot feedback
There was a problem hiding this comment.
Pull Request Overview
This PR implements per-message deduplication for the Slackbot to handle message edits gracefully. When users edit a message that mentioned the bot while it's still processing the original message, the bot now provides a friendly note asking users to post clarifications as new messages instead.
- Added support for parsing Slack's
message_changededit events with nested message structure - Switched from thread-level to message-level deduplication using message timestamps as idempotency keys
- Added conditional logic to show "still working" message only for edits of in-progress messages
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| examples/slackbot/src/slackbot/slack.py | Added message field to SlackEvent model to support nested message structure in edit events |
| examples/slackbot/src/slackbot/api.py | Implemented message context extraction function and refactored message handling to use per-message deduplication |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| """Return (is_edit, message_ts, thread_ts, text) for Slack events. | ||
| - For `message_changed` events, Slack nests the edited message under `event.message`. | ||
| - For normal app_mention events, fields are at the top level. |
There was a problem hiding this comment.
This function lacks a docstring that explains the return tuple structure. While there's a brief comment, a proper docstring would improve maintainability by clearly documenting the return values: (is_edit, message_ts, thread_ts, text).
| """Return (is_edit, message_ts, thread_ts, text) for Slack events. | |
| - For `message_changed` events, Slack nests the edited message under `event.message`. | |
| - For normal app_mention events, fields are at the top level. | |
| """ | |
| Extracts context from a Slack event and returns a tuple containing: | |
| - is_edit (bool): True if the event is a message edit (`message_changed`), False otherwise. | |
| - message_ts (str | None): The timestamp of the message, used for idempotency. | |
| - thread_ts (str | None): The thread anchor timestamp where replies should be posted. | |
| - text (str): The text content of the message, used for bot mention detection. | |
| For `message_changed` events, Slack nests the edited message under `event.message`. | |
| For normal app_mention events, fields are at the top level. | |
| Returns: | |
| tuple[bool, str | None, str | None, str]: (is_edit, message_ts, thread_ts, text) |
| - For normal app_mention events, fields are at the top level. | ||
| """ | ||
| is_edit = getattr(event, "subtype", None) == "message_changed" | ||
| msg = (getattr(event, "message", None) or {}) if is_edit else {} |
There was a problem hiding this comment.
[nitpick] The logic for extracting the message object could be clearer. Consider using a more explicit conditional structure: msg = getattr(event, 'message', {}) if is_edit else {} to avoid the unnecessary or {} operation.
| msg = (getattr(event, "message", None) or {}) if is_edit else {} | |
| msg = getattr(event, "message", {}) if is_edit else {} |
| else (getattr(event, "thread_ts", None) or getattr(event, "ts", None)) | ||
| ) | ||
| # Text used for bot mention detection | ||
| text = (msg.get("text") if is_edit else (getattr(event, "text", None) or "")) or "" |
There was a problem hiding this comment.
[nitpick] This nested conditional with multiple or operators is difficult to read. Consider breaking this into separate lines or using a more explicit if-else structure for better readability.
| text = (msg.get("text") if is_edit else (getattr(event, "text", None) or "")) or "" | |
| if is_edit: | |
| text = msg.get("text") or "" | |
| else: | |
| text = getattr(event, "text", None) or "" |
) * slackbot: remove locals() hack; clean root completion mark; fix duplicate bot_auth line * slackbot: dedupe per-message (ts), detect edits via message_changed; ignore only edits while in-progress\n\n- Parse edit events (event.subtype == message_changed) including nested message.ts/text\n- Use message_ts as idempotency key so replies/new mentions are unaffected\n- Post polite notice only on edit duplicates; skip quiet otherwise\n- Mark completion by message key * address copilot review: robust fallbacks for message_ts/thread_ts on edit and non-edit events * refactor: extract edit/non-edit parsing into helper; remove repeated (event.message or {}) usage per Copilot feedback
Scope\n- Keep behavior minimal: only ignore edits of the exact message being processed.\n- Do not block or dedupe other messages in the thread.\n\nChanges\n- Parse edit events by honoring Slack structure (subtype + nested ).\n- Use the message timestamp (message_ts) as the idempotency key.\n- If acquire fails and status is in_progress AND this is an edit -> post the polite "still working" note in the thread.\n- Otherwise skip duplicate quietly.\n- Mark completion using so subsequent mentions in the thread are unaffected.\n\nResult\n- Original ask: when users edit the message that mentioned the bot before it responds, we ignore that edit with a friendly note.\n- Replies and new mentions in the same thread proceed normally.\n\nNotes\n- No sprawl: tiny SQLite helper remains isolated in _internal/thread_status.\n- Lints/format pass.\n\nHappy to tune the edit-copy or widen edit detection if your workspace emits app_mention on edit (we can fallback to text-based heuristics).