Fix Slack DM integration event delivery#116
Merged
Merged
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. |
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. 📝 WalkthroughWalkthroughThis PR enhances Slack event integration by adding content-aware deduplication with logical change fingerprinting, refactoring event context preview generation with Slack-specific extraction, and implementing specialized Slack message formatting. It also refactors system message deduplication from TTL-based to content-based tracking and reorders local mount daemon synchronization. ChangesSlack Integration Event Handling and System Message Deduplication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
codeant-ai
Bot
added
the
size:XL
Contributor
|
Reviewed PR #116 against the checked-out diff and traced the changed integration-event and integration guidance paths. I did not make code changes because the current checkout did not present a reproducible defect. Verified:
All passed. |
Contributor
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. Reviewed PR #116 against the checked-out diff and traced the changed integration-event and integration guidance paths. I did not make code changes because the current checkout did not present a reproducible defect. Verified:
All passed. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+940
to
+944
| void this.syncLocalMounts() | ||
| .then(() => this.syncActiveEventSubscriptions()) | ||
| .catch((error) => { | ||
| console.warn('[integrations] Failed to start local integration mounts:', toErrorMessage(error)) | ||
| }) |
There was a problem hiding this comment.
Suggestion: This fire-and-forget startup chain can continue running after shutdown starts, and the trailing syncActiveEventSubscriptions() can re-open subscriptions that shutdownLocalMounts() just closed. Track/cancel startup work (or await it under lifecycle control) so shutdown cannot race with a late re-subscribe. [race condition]
Severity Level: Major ⚠️
- ⚠️ Event subscriptions can briefly reopen during app shutdown.
- ⚠️ Integration websocket/file watchers may linger after cleanup.
- ⚠️ Shutdown ordering becomes non-deterministic and harder to reason.Steps of Reproduction ✅
1. On app startup, `src/main/index.ts:329-331` calls `void
integrationsManager.startLocalMountDaemon().catch(...)`, starting background integration
initialization without awaiting it.
2. `startLocalMountDaemon()` at `src/main/integrations.ts:938-945` first awaits
`this.syncActiveEventSubscriptions()` (line 939), then kicks off
`this.syncLocalMounts().then(() => this.syncActiveEventSubscriptions())` as a
fire-and-forget chain (lines 940-942).
3. If the user initiates app shutdown while `syncLocalMounts()` is still running (e.g.
soon after launch while mounts or cloud hydration at `integrations.ts:1885-1949` are in
progress), the shutdown handler in `src/main/index.ts:350-368` calls
`integrationsManager.shutdownLocalMounts()` as part of `Promise.allSettled([...])`.
4. `shutdownLocalMounts()` at `src/main/integrations.ts:955-969` clears system-message
timers and then awaits both `integrationMountManager.stop()` and
`integrationEventBridge.closeAll()`, closing all integration event subscriptions.
5. After `shutdownLocalMounts()` resolves, the original `syncLocalMounts()` call (started
in step 2) may complete; its `.then(() => this.syncActiveEventSubscriptions())` callback
(lines 940-942) then runs even though shutdown is in progress or complete.
6. That late `syncActiveEventSubscriptions()` call at `integrations.ts:1876-1883` reads
the still-set `activeProjectId` from the store and invokes
`syncEventSubscriptions(activeProjectId)` (lines 1877-1882), which performs
`integrationEventBridge.closeAllExcept(projectId)` and
`integrationEventBridge.reconcile(projectId, ...)` at `integrations.ts:1858-1868`.
7. As a result, integration event subscriptions may be re-opened after
`shutdownLocalMounts()` has already closed them, purely due to this untracked
fire-and-forget startup chain racing with the shutdown lifecycle.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/integrations.ts
**Line:** 940:944
**Comment:**
*Race Condition: This fire-and-forget startup chain can continue running after shutdown starts, and the trailing `syncActiveEventSubscriptions()` can re-open subscriptions that `shutdownLocalMounts()` just closed. Track/cancel startup work (or await it under lifecycle control) so shutdown cannot race with a late re-subscribe.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (expiresAt <= now) this.recentlyInjectedSystemMessages.delete(key) | ||
| } | ||
| if (this.recentlyInjectedSystemMessages.has(messageKey)) return | ||
| if (this.lastBroadcastSystemMessages.get(projectId) === message) return |
There was a problem hiding this comment.
Suggestion: The dedupe check is done before looking up current recipients, so once a message body has been sent once for a project, later calls skip delivery entirely even if a different/new agent is now online. Move dedupe tracking to be recipient-aware (or at least re-evaluate agent set before short-circuiting) so newly joined agents still receive the current integrations update. [logic error]
Severity Level: Major ⚠️
- ⚠️ New broker sessions miss `<integrations-update>` system messages.
- ⚠️ Newly spawned agents may lack current integration context.
- ⚠️ Integrations guidance diverges from actual connected integrations.Steps of Reproduction ✅
1. App startup registers IPC handler `broker:start` at `src/main/ipc-handlers.ts:58-72`,
which on success calls `integrationsManager.notifyAgentState(projectId)` (lines 65-69).
2. Renderer triggers a broker start for a project (e.g. user starts the local assistant),
invoking `broker:start`, which calls `notifyAgentState()` at
`src/main/integrations.ts:947-949`.
3. `notifyAgentState()` calls `syncAgentState(projectId, true, { waitForAgent: true })`
(`integrations.ts:946-950`), which in turn calls `scheduleSystemMessage()` at
`integrations.ts:1655-1659`. After debounce, `scheduleSystemMessage()` calls
`safeInjectSystemMessage(projectId, message, options)` at `integrations.ts:1841-1844`.
4. On the first run, `safeInjectSystemMessage()` at `integrations.ts:1794-1827` sends the
`<integrations-update>` message to all current agents and then records
`this.lastBroadcastSystemMessages.set(projectId, message)` at line 1827.
5. Later, the user restarts the broker for the same project with unchanged integrations
(same `buildSystemMessageSnippet` output), so `broker:start` again invokes
`notifyAgentState()` → `scheduleSystemMessage()` → `safeInjectSystemMessage()`.
6. On this second run, `safeInjectSystemMessage()` hits the dedupe guard at
`integrations.ts:1800` (`if (this.lastBroadcastSystemMessages.get(projectId) === message)
return`) and exits before calling `listSystemMessageAgents()` or `bridge.sendMessage()`,
so any newly-started agents for the project never receive the current integrations-update
message.
7. `lastBroadcastSystemMessages` is never cleared or made recipient-aware (verified via
`grep` at `integrations.ts:722-725` and `integrations.ts:1827` being the only writes), so
this suppression persists for the lifetime of the main process as long as the message body
is unchanged.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/integrations.ts
**Line:** 1800:1800
**Comment:**
*Logic Error: The dedupe check is done before looking up current recipients, so once a message body has been sent once for a project, later calls skip delivery entirely even if a different/new agent is now online. Move dedupe tracking to be recipient-aware (or at least re-evaluate agent set before short-circuiting) so newly joined agents still receive the current integrations update.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/main/integrations.test.ts`:
- Around line 486-498: The mock call history is cleared with
mock.brokerManager.sendMessage.mockClear(), so subsequent assertions should
expect zero new sends; update the two assertions after calls to
manager.notifyAgentState('project-1') (the expectations using
integrationsUpdateSends()) to expect 0 instead of 1 to reflect dedupe working,
keeping the notifyAgentState and vi.advanceTimersByTimeAsync calls unchanged.
- Around line 582-586: The test races because it asserts
integrationEventBridge.reconcile call counts immediately after waiting for
integrationMountManager.ensureMounted; change the test to waitFor the reconcile
behavior instead: use vi.waitFor(() =>
expect(mock.integrationEventBridge.reconcile).toHaveBeenCalledTimes(2)) (or
waitFor the specific toHaveBeenLastCalledWith) before asserting
expect(mock.integrationEventBridge.closeAllExcept).toHaveBeenCalledTimes(2) and
the last-call arguments; this ensures integrationEventBridge.reconcile (and its
chained async step) has completed before making deterministic assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90b58355-6b16-4d0e-8d72-69f52fed27ae
📒 Files selected for processing (5)
src/main/__tests__/integration-event-bridge.test.tssrc/main/cloud-agent.tssrc/main/integration-event-bridge.tssrc/main/integrations.test.tssrc/main/integrations.ts
Comment on lines
+486
to
+498
| mock.brokerManager.sendMessage.mockClear() | ||
|
|
||
| // A later reconcile with identical integration state must not re-broadcast | ||
| // regardless of elapsed time — duplicate <integrations-update> messages | ||
| // were observed minutes apart with the old 30s text-TTL dedupe. | ||
| await manager.notifyAgentState('project-1') | ||
| await vi.advanceTimersByTimeAsync(1_000) | ||
| expect(integrationsUpdateSends()).toBe(1) | ||
|
|
||
| await vi.advanceTimersByTimeAsync(60_000) | ||
| await manager.notifyAgentState('project-1') | ||
| await vi.advanceTimersByTimeAsync(1_000) | ||
| expect(integrationsUpdateSends()).toBe(1) |
There was a problem hiding this comment.
Fix dedupe assertions after clearing the mock call history.
After mock.brokerManager.sendMessage.mockClear() on Line 486, the assertions on Lines 493 and 498 should expect 0 new sends (dedupe working), not 1.
Suggested fix
- expect(integrationsUpdateSends()).toBe(1)
+ expect(integrationsUpdateSends()).toBe(0)
@@
- expect(integrationsUpdateSends()).toBe(1)
+ expect(integrationsUpdateSends()).toBe(0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mock.brokerManager.sendMessage.mockClear() | |
| // A later reconcile with identical integration state must not re-broadcast | |
| // regardless of elapsed time — duplicate <integrations-update> messages | |
| // were observed minutes apart with the old 30s text-TTL dedupe. | |
| await manager.notifyAgentState('project-1') | |
| await vi.advanceTimersByTimeAsync(1_000) | |
| expect(integrationsUpdateSends()).toBe(1) | |
| await vi.advanceTimersByTimeAsync(60_000) | |
| await manager.notifyAgentState('project-1') | |
| await vi.advanceTimersByTimeAsync(1_000) | |
| expect(integrationsUpdateSends()).toBe(1) | |
| mock.brokerManager.sendMessage.mockClear() | |
| // A later reconcile with identical integration state must not re-broadcast | |
| // regardless of elapsed time — duplicate <integrations-update> messages | |
| // were observed minutes apart with the old 30s text-TTL dedupe. | |
| await manager.notifyAgentState('project-1') | |
| await vi.advanceTimersByTimeAsync(1_000) | |
| expect(integrationsUpdateSends()).toBe(0) | |
| await vi.advanceTimersByTimeAsync(60_000) | |
| await manager.notifyAgentState('project-1') | |
| await vi.advanceTimersByTimeAsync(1_000) | |
| expect(integrationsUpdateSends()).toBe(0) |
🤖 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 `@src/main/integrations.test.ts` around lines 486 - 498, The mock call history
is cleared with mock.brokerManager.sendMessage.mockClear(), so subsequent
assertions should expect zero new sends; update the two assertions after calls
to manager.notifyAgentState('project-1') (the expectations using
integrationsUpdateSends()) to expect 0 instead of 1 to reflect dedupe working,
keeping the notifyAgentState and vi.advanceTimersByTimeAsync calls unchanged.
Comment on lines
+582
to
+586
| await vi.waitFor(() => expect(mock.integrationMountManager.ensureMounted).toHaveBeenCalled()) | ||
|
|
||
| expect(mock.integrationEventBridge.closeAllExcept).toHaveBeenCalledTimes(2) | ||
| expect(mock.integrationEventBridge.reconcile).toHaveBeenCalledTimes(2) | ||
| expect(mock.integrationEventBridge.reconcile).toHaveBeenLastCalledWith( |
There was a problem hiding this comment.
Make the startup retry assertion deterministic.
The test waits for ensureMounted but immediately asserts two reconcile calls. Since the second reconcile runs in a chained async step, this can intermittently race.
Suggested fix
- await vi.waitFor(() => expect(mock.integrationMountManager.ensureMounted).toHaveBeenCalled())
-
- expect(mock.integrationEventBridge.closeAllExcept).toHaveBeenCalledTimes(2)
- expect(mock.integrationEventBridge.reconcile).toHaveBeenCalledTimes(2)
+ await vi.waitFor(() => expect(mock.integrationMountManager.ensureMounted).toHaveBeenCalled())
+ await vi.waitFor(() => expect(mock.integrationEventBridge.closeAllExcept).toHaveBeenCalledTimes(2))
+ await vi.waitFor(() => expect(mock.integrationEventBridge.reconcile).toHaveBeenCalledTimes(2))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await vi.waitFor(() => expect(mock.integrationMountManager.ensureMounted).toHaveBeenCalled()) | |
| expect(mock.integrationEventBridge.closeAllExcept).toHaveBeenCalledTimes(2) | |
| expect(mock.integrationEventBridge.reconcile).toHaveBeenCalledTimes(2) | |
| expect(mock.integrationEventBridge.reconcile).toHaveBeenLastCalledWith( | |
| await vi.waitFor(() => expect(mock.integrationMountManager.ensureMounted).toHaveBeenCalled()) | |
| await vi.waitFor(() => expect(mock.integrationEventBridge.closeAllExcept).toHaveBeenCalledTimes(2)) | |
| await vi.waitFor(() => expect(mock.integrationEventBridge.reconcile).toHaveBeenCalledTimes(2)) | |
| expect(mock.integrationEventBridge.reconcile).toHaveBeenLastCalledWith( |
🤖 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 `@src/main/integrations.test.ts` around lines 582 - 586, The test races because
it asserts integrationEventBridge.reconcile call counts immediately after
waiting for integrationMountManager.ensureMounted; change the test to waitFor
the reconcile behavior instead: use vi.waitFor(() =>
expect(mock.integrationEventBridge.reconcile).toHaveBeenCalledTimes(2)) (or
waitFor the specific toHaveBeenLastCalledWith) before asserting
expect(mock.integrationEventBridge.closeAllExcept).toHaveBeenCalledTimes(2) and
the last-call arguments; this ensures integrationEventBridge.reconcile (and its
chained async step) has completed before making deterministic assertions.
|
CodeAnt AI finished reviewing your PR. |
This was referenced Jun 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
D0B...are no longer dropped before Pear sees themRoot Cause
Pear subscribed to DM events with
/slack/channels/D*/**, but the Relayfile SDK transport filter only supports*as a whole segment plus trailing**. The SDK therefore treatedD*as a literal segment and filtered out/slack/channels/D0B...events before Pear's own matcher could inject them.Tests
npm testCodeAnt-AI Description
Fix Slack event delivery and make integration updates more reliable
What Changed
Impact
✅ Fewer missing Slack DM notifications✅ No duplicate Slack message alerts from alias paths✅ Clearer Slack event context in agent messages💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.