Notify a thread's participants on every new comment, debounced - #204
Conversation
…ment Every comment now fires the comment_created event, so a reply notifies everyone already in the thread, not only the plan owner on the first comment. SlackNotificationJob is debounced per-thread so a burst of comments (e.g. an agent posting several in a row) collapses into one Slack DM per recipient instead of one per comment, and a permanent failure for one recipient no longer blocks the rest of the batch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe18dd8484
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pending_key = pending_key(comment_thread_id) | ||
| return if Rails.cache.read(pending_key) | ||
|
|
||
| Rails.cache.write(pending_key, true, expires_in: CACHE_EXPIRY) |
There was a problem hiding this comment.
When notification workers process two comments from the same thread concurrently, both can read a missing pending key before either writes it, so both enqueue delayed jobs. Those jobs later select the same batch and send duplicate DMs, defeating the debounce precisely for concurrent bursts. Use an atomic cache claim such as a write with unless_exist: true and enqueue only for the caller that acquired it.
Useful? React with 👍 / 👎.
| recipients.each do |user| | ||
| next unless user.email.present? | ||
| send_dm(user, text) |
There was a problem hiding this comment.
With multiple recipients, a transient Slack error after one or more successful sends raises out of this loop and retry_on reruns the whole job. Every recipient processed before the failure receives the same DM again on each retry, so a rate limit or temporary error for one user can spam the other participants. Preserve the failed recipient set or dispatch independently retryable sends per recipient.
Useful? React with 👍 / 👎.
- batch_start was Time.current at debounce-call time, which is always after the triggering comment's own created_at (the comment must exist to have enqueued the job that calls debounce). That silently excluded the triggering comment from its own notification. Now passed through from the comment's created_at instead. - Debounce's pending-flag check-and-set was read-then-write, not atomic; two concurrent calls could both schedule a job. Use Rails.cache's unless_exist: true instead. - The pending flag was cleared at the top of perform, before any work happened, so a comment arriving mid-retry could start an overlapping batch that clobbered this one's cached batch_start. Now only cleared after a full successful send. - A transient error partway through the recipient loop caused the whole job to retry, re-sending to recipients who'd already succeeded. Track already-notified recipients in cache and skip them on retry. - Align soft-delete scoping: recipients_for now uses .kept like the new_comments query, so a participant whose only comment was deleted doesn't stay a permanent participant asymmetrically.
1 participant
Summary
comment_creatednotification event, not just the thread's first comment — replies now notify the thread's participants, not only the plan owner on thread creation.SlackNotificationJobis debounced per-thread (2-minute window): a burst of comments (e.g. an agent posting several in a row) collapses into one Slack DM per recipient instead of one per comment.Test plan
bundle exec rspec— full suite green (1805 examples)bundle exec rubocopon touched files — clean🤖 Generated with Claude Code