feat(connector-slack): @peek in a channel — app_mention + threaded auto-continue#155
Conversation
…to-continue Adds an app_mention handler (threaded, natural-language answers visible to the channel), scopes app.message to DMs + peek's active threads (firehose fix), and dedupes the mention/message double-event. Makes shared-channel debugging first-class. No runtime/consent change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
…y-botId coverage Fix 1: getBoltHandler now throws a descriptive Error on out-of-range index or a missing/empty chain or null callback, converting previously silent test mis-wiring into a loud failure. A new "Bolt internal listener layout tripwire" test asserts boltApp.listeners.length === 5; a Bolt version bump that shifts the internal layout will fail this assertion rather than silently routing subsequent handler calls to the wrong index. Fix 2 (comment only, no logic change): Added a one-line comment on the idempotent #activeThreads.add(cid) keep-alive inside the app.message handler in slack-adapter.ts. Fix 3: New test locks the empty-botUserId branch of the app.message dedupe guard: when botUserId is absent/empty the botId && ... check short-circuits and skips the dedupe, so a DM whose text contains a mention token falls through to the isDM gate and emits normally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an exported ChangesSlack mention stripping and thread activation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Slack
participant BoltApp
participant SlackAdapter
Slack->>BoltApp: app_mention event
BoltApp->>SlackAdapter: stripMention(text, botUserId)
SlackAdapter->>SlackAdapter: activeThreads.add(threadId)
SlackAdapter->>SlackAdapter: emit(strippedQuery)
Slack->>BoltApp: message event
BoltApp->>SlackAdapter: evaluate channel_type / activeThreads / mention dedupe
alt DM or active thread, not deduped
SlackAdapter->>SlackAdapter: emit(message)
else suppressed
SlackAdapter-->>BoltApp: ignore
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (3)
packages/connector-slack/src/slack-adapter.ts (3)
37-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer
String.replaceAllovernew RegExpfor literal token replacement.
botUserIdcomes from Slack's API (always alphanumeric), so the ReDoS risk is negligible — butreplaceAllwith a string literal eliminates the regex construction entirely, is simpler, and avoids any future injection surface if the input source ever changes.♻️ Proposed refactor
export function stripMention(text: string, botUserId: string): string { return text - .replace(new RegExp(`<@${botUserId}>`, 'g'), ' ') + .replaceAll(`<@${botUserId}>`, ' ') .replace(/\s+/g, ' ') .trim(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/connector-slack/src/slack-adapter.ts` around lines 37 - 38, The token replacement in Slack text normalization should avoid building a RegExp for a literal mention string. Update the replacement logic in the Slack adapter’s text-processing path to use String.replaceAll with the constructed bot mention token instead of new RegExp, keeping the behavior of replacing all occurrences with a space while removing the regex dependency.
63-63: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
#activeThreadsgrows without bound — consider a cleanup mechanism.Entries are added on every
app_mention(line 215) and thread reply (line 237) but never removed. For the alpha this is fine, but a long-running socket-mode process will accumulate stale thread IDs indefinitely, and threads activated months ago will still trigger the bot on any reply. A TTL-based eviction (e.g. remove entries inactive for N minutes) or a max-size cap would be a low-effort guard for when this moves beyond alpha.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/connector-slack/src/slack-adapter.ts` at line 63, `#activeThreads` in `SlackAdapter` is only ever added to and never pruned, so the set can grow indefinitely and keep stale thread IDs active. Add a cleanup mechanism around the `handleMention`/thread-reply path in `slack-adapter.ts` (where `#activeThreads` is updated) such as TTL-based expiration or a bounded-size eviction strategy, and make sure the tracking structure removes inactive entries automatically over time.
237-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 237 is a no-op — consider removing.
inActiveThreadguaranteesthis.#activeThreads.has(m.thread_ts)is true andcid === m.thread_ts, sothis.#activeThreads.add(cid)always re-adds an existing entry. The comment acknowledges this, but the line adds cognitive overhead for future readers without behavioral value.♻️ Proposed refactor
- // Idempotent no-op when cid is already in the set — kept as a defensive keep-alive for threaded replies. - if (inActiveThread) this.#activeThreads.add(cid); this.emit(cid, m.channel, cid, m.user ?? 'unknown', m.text);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/connector-slack/src/slack-adapter.ts` at line 237, The `SlackAdapter` thread-handling logic contains a no-op in the `inActiveThread` branch: `this.#activeThreads.add(cid)` re-adds an already tracked thread ID and adds no behavior. Remove that statement from the `SlackAdapter` flow around the `inActiveThread` check, and keep the surrounding thread state logic unchanged so the intent remains clear in `SlackAdapter` and its `#activeThreads` handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/connector-slack/src/slack-adapter.ts`:
- Around line 229-234: The DM handling in slack-adapter.ts is dropping messages
because the mention dedupe check runs before determining whether the message is
a DM. Update the logic in the Slack message handler so the `<`@botId`>`
suppression only applies when `isDM` is false, and keep the existing
`inActiveThread` and channel-chatter filtering in the same flow within the
relevant message-processing path.
---
Nitpick comments:
In `@packages/connector-slack/src/slack-adapter.ts`:
- Around line 37-38: The token replacement in Slack text normalization should
avoid building a RegExp for a literal mention string. Update the replacement
logic in the Slack adapter’s text-processing path to use String.replaceAll with
the constructed bot mention token instead of new RegExp, keeping the behavior of
replacing all occurrences with a space while removing the regex dependency.
- Line 63: `#activeThreads` in `SlackAdapter` is only ever added to and never
pruned, so the set can grow indefinitely and keep stale thread IDs active. Add a
cleanup mechanism around the `handleMention`/thread-reply path in
`slack-adapter.ts` (where `#activeThreads` is updated) such as TTL-based
expiration or a bounded-size eviction strategy, and make sure the tracking
structure removes inactive entries automatically over time.
- Line 237: The `SlackAdapter` thread-handling logic contains a no-op in the
`inActiveThread` branch: `this.#activeThreads.add(cid)` re-adds an already
tracked thread ID and adds no behavior. Remove that statement from the
`SlackAdapter` flow around the `inActiveThread` check, and keep the surrounding
thread state logic unchanged so the intent remains clear in `SlackAdapter` and
its `#activeThreads` handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 40b1c0ab-258d-4b16-8f73-646ceeecdc31
📒 Files selected for processing (2)
packages/connector-slack/src/slack-adapter.test.tspackages/connector-slack/src/slack-adapter.ts
…laceAll, no-op removal, thread cap Fix 1 (blocker): compute isDM before the mention-dedupe guard and gate it on !isDM. A DM whose text contains <@botId> was silently dropped because Slack never emits app_mention for DMs, leaving nothing to handle it. Fix 2: stripMention uses String.replaceAll (string literal) instead of `new RegExp(..., 'g')` — same semantics, no regex construction. Fix 3: remove the `if (inActiveThread) this.#activeThreads.add(cid)` no-op in app.message (when inActiveThread is true, cid is already in the set). Fix 4: bound #activeThreads growth with a 1000-entry insertion-order cap via a new #trackThread() private helper; app_mention calls #trackThread instead of the bare .add(). TTL deferred past alpha per CodeRabbit acknowledgement. Tests: regression for Fix 1 (DM + non-empty botUserId + mention token → emits), observable-behavior cap test for Fix 4 (oldest evicted, recent alive), updated empty-botUserId test comment to reflect new guard shape. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: harry-harish <22562634+harry-harish@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What
Makes shared-channel debugging first-class in the peek Slack connector. Today peek answers via the per-user Assistant pane, a DM, and
/peek— none of which give the "natural-language + shared-channel + threaded, visible to the whole team" combo that shared debugging needs.app_mentionhandler —@peek what failed in my last session?in a channel → the mention is stripped and routed to the same runtime the assistant/DM use, and peek answers in a thread under the mention (visible to the channel).app.messagefirehose fix — the old catch-all answered every message in any channel peek was in. It's now scoped to DMs + peek's active threads.app_mentionandmessage; the message handler skips text containing the bot mention so each is processed exactly once.The assistant pane,
/peek, and the consent/approve/deny paths are unchanged. Acting from a channel thread works via the existing consent card (any channel member may approve in v1; the destructive backstop still runs on the host).Tests
stripMention(pure helper) + theapp_mentionandapp.messagehandlers, covering all five message paths (top-level mention, threaded follow-up, unrelated channel message, DM, second in-thread mention) — plus a loud tripwire asserting the Bolt internal listener layout, so a future Bolt bump fails the tests instead of silently mistesting. 58 passing.Notes
@peekdev/connector-slackisprivate: true.app_mentionbot event + add theapp_mentions:readscope, then reinstall.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes