-
Notifications
You must be signed in to change notification settings - Fork 385
Slackbot: ignore edits while initial response is in progress #1224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ess lock)\n\n- Add simple per-thread asyncio.Lock to gate concurrent handling\n- Send friendly notice for message_changed edits during processing\n- Avoid DB changes; keep example lightweight\n- Add subtype to SlackEvent model\n
…nal.thread_status with try_acquire/get_status/mark_completed\n- Gate mention handling with DB-backed idempotency key (thread_ts)\n- Post friendly edit-ignored message when in_progress\n- Keep api/core lean; no sprawl
…sert message for edit notice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements cross-process deduplication for Slack events to prevent duplicate responses when users edit their original messages while the bot is processing. The solution uses a lightweight SQLite table to track thread processing status.
- Add thread status tracking with in_progress/completed states stored in SQLite
- Implement cross-process acquire mechanism using atomic INSERT OR IGNORE operations
- Send polite notice to users when edits are detected during processing
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| examples/slackbot/src/slackbot/slack.py | Adds optional subtype field to SlackEvent model for future edit detection |
| examples/slackbot/src/slackbot/core.py | Adds comment explaining thread status is handled in api.py |
| examples/slackbot/src/slackbot/api.py | Integrates thread status checking and user notification logic into message handler |
| examples/slackbot/src/slackbot/_internal/thread_status.py | New module providing SQLite-backed thread status tracking functions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| (thread_ts,), | ||
| ) | ||
| row = cur.fetchone() | ||
| return row[0] if row else None # type: ignore[return-value] |
Copilot
AI
Sep 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type ignore comment is unnecessary. The return type annotation already handles the None case correctly, and mypy should understand that row[0] will be a Status when row is not None.
| return row[0] if row else None # type: ignore[return-value] | |
| return row[0] if row else None |
| assert event.channel is not None, ( | ||
| "Event channel is None when posting edit-ignored notice" | ||
| ) |
Copilot
AI
Sep 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion message should be more descriptive and actionable. Consider changing to 'Event channel cannot be None when posting notification message' or use proper error handling instead of assertion.
| assert event.channel is not None, ( | |
| "Event channel is None when posting edit-ignored notice" | |
| ) | |
| if event.channel is None: | |
| logger.error("Event channel is None when posting edit-ignored notice") | |
| return Completed( | |
| message="Cannot post edit-ignored notice: event channel is None", | |
| name="SKIPPED_NO_CHANNEL", | |
| data=dict(thread_ts=root_ts), | |
| ) |
| try: | ||
| await mark_thread_completed(db, root_ts) | ||
| except Exception: | ||
| logger.warning("Failed to mark thread as completed") |
Copilot
AI
Sep 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning message should include the specific error details for better debugging. Consider logging the exception: logger.warning('Failed to mark thread as completed', exc_info=True) or logger.warning('Failed to mark thread as completed: %s', e).
| logger.warning("Failed to mark thread as completed") | |
| logger.warning("Failed to mark thread as completed", exc_info=True) |
Summary\n- Prevent duplicate responses when users edit their original mention mid-run\n- Cross-process dedupe via tiny sqlite status in _internal.thread_status (idempotency key = root thread_ts)\n- If an edit arrives while in progress, post a polite notice asking for follow-ups as new thread messages\n- Keep app logic lean: minimal changes to api.py; no changes to core DB class\n\nDesign\n- Adds _internal/thread_status.py with ensure_schema/try_acquire/get_status/mark_completed. Schema is created lazily.\n- api.py acquires per-thread key before processing; on conflict, checks status: in_progress => send edit-ignored message; completed => skip.\n- Uses the existing sqlite DB file from settings; no new service or dep.\n- Adds optional SlackEvent.subtype for future-proof edit detection; otherwise behavior is based on duplicate events regardless of subtype.\n\nNotes\n- Keeps example scope small and avoids sprawl into core.\n- Passes ruff and format hooks.\n- Happy to adjust the copy for the polite notice or extend dedupe keying if you want to incorporate client_msg_id.\n