Fix integration event local previews#159
Conversation
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
📝 WalkthroughWalkthroughThe PR adds Slack writeback draft cleanup triggered on confirmed writebacks, exports a previously internal ChangesSlack Writeback Cleanup and LocalMountPaths Threading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces local draft cleanup for confirmed Slack writeback events and ensures current local mount paths are passed during integration event reconciliation. Feedback on these changes highlights two issues in the draft cleanup logic: first, the check isSlackWritebackCommandRoot(root.remoteRoot) can incorrectly bypass cleanup when downloadHistoricalData is enabled; second, returning early after the first successful deletion prevents the cleanup of duplicate draft files across other matching local mount roots.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!isSlackWritebackCommandRoot(root.remoteRoot)) continue | ||
| if (!pathIsInsideMount(remotePath, root.remoteRoot)) continue |
There was a problem hiding this comment.
When downloadHistoricalData is enabled (true), the root.remoteRoot is a broader history root (e.g., /slack/channels/C123) rather than the narrow command root (e.g., /slack/channels/C123/messages). This causes isSlackWritebackCommandRoot(root.remoteRoot) to return false, completely bypassing the cleanup of confirmed writeback drafts for users with historical download enabled.\n\nSince isLikelyLocalWritebackCommandPath(remotePath) and pathIsInsideMount(remotePath, root.remoteRoot) already ensure the path is a valid writeback command file within the mount, this extra check is unnecessary and buggy.
| if (!isSlackWritebackCommandRoot(root.remoteRoot)) continue | |
| if (!pathIsInsideMount(remotePath, root.remoteRoot)) continue | |
| if (!pathIsInsideMount(remotePath, root.remoteRoot)) continue |
| logIntegrationEvent('confirmed Slack writeback draft cleaned', { | ||
| projectId, | ||
| eventId: event.id, | ||
| remotePath, | ||
| localRoot: root.localRoot | ||
| }) | ||
| return |
There was a problem hiding this comment.
Returning early after the first successful deletion prevents the cleanup of duplicate draft files in other matching local mount roots (for example, if the same channel is mounted in multiple local workspaces or roots). Removing the return statement allows the loop to continue and clean up all matching local draft files.
logIntegrationEvent('confirmed Slack writeback draft cleaned', {\n projectId,\n eventId: event.id,\n remotePath,\n localRoot: root.localRoot\n })![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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/__tests__/integration-event-bridge.test.ts`:
- Around line 2117-2177: Add a duplicate/replay check to the "confirmed Slack
writeback success removes the local draft command file" test: after the first
harness.emit of the writeback.succeeded ChangeEvent (remoteDraftPath) and
waitForPathMissing, emit the identical writeback.succeeded event again via
harness.emit (same ChangeEvent payload) and assert the local draft still does
not exist (waitForPathMissing or stat check) and harness.sent remains [] to
ensure idempotent cleanup; update assertions around localDraftPath,
remoteDraftPath, harness.emit and waitForPathMissing to reflect the replayed
event case.
- Around line 154-162: The helper waitForPathMissing currently swallows all
errors from stat (catch(() => null)), which treats permission/IO errors as
“missing”; change the catch to only treat ENOENT as missing and rethrow any
other errors: in waitForPathMissing wrap the stat call with a catch that checks
err.code === 'ENOENT' then returns null, otherwise rethrow the error so tests
fail on non-ENOENT issues; reference the waitForPathMissing function and the
stat call to locate the change.
🪄 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: 26c5476e-ddea-4f82-b9b5-cd54c52a2184
📒 Files selected for processing (4)
src/main/__tests__/integration-event-bridge.test.tssrc/main/integration-event-bridge.tssrc/main/integrations.test.tssrc/main/integrations.ts
| async function waitForPathMissing(path: string, timeoutMs = 2_000): Promise<void> { | ||
| const deadline = Date.now() + timeoutMs | ||
| while (Date.now() <= deadline) { | ||
| const stats = await stat(path).catch(() => null) | ||
| if (!stats) return | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| } | ||
| assert.equal(await stat(path).then(() => true).catch(() => false), false) | ||
| } |
There was a problem hiding this comment.
Avoid swallowing non-ENOENT errors in waitForPathMissing.
On Line 157, catch(() => null) converts every stat failure into “missing,” which can falsely pass tests on permission/IO errors.
Suggested fix
async function waitForPathMissing(path: string, timeoutMs = 2_000): Promise<void> {
const deadline = Date.now() + timeoutMs
while (Date.now() <= deadline) {
- const stats = await stat(path).catch(() => null)
+ const stats = await stat(path).catch((error: NodeJS.ErrnoException) => {
+ if (error?.code === 'ENOENT') return null
+ throw error
+ })
if (!stats) return
await new Promise((resolve) => setTimeout(resolve, 10))
}
- assert.equal(await stat(path).then(() => true).catch(() => false), false)
+ const exists = await stat(path)
+ .then(() => true)
+ .catch((error: NodeJS.ErrnoException) => {
+ if (error?.code === 'ENOENT') return false
+ throw error
+ })
+ assert.equal(exists, false)
}📝 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.
| async function waitForPathMissing(path: string, timeoutMs = 2_000): Promise<void> { | |
| const deadline = Date.now() + timeoutMs | |
| while (Date.now() <= deadline) { | |
| const stats = await stat(path).catch(() => null) | |
| if (!stats) return | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| } | |
| assert.equal(await stat(path).then(() => true).catch(() => false), false) | |
| } | |
| async function waitForPathMissing(path: string, timeoutMs = 2_000): Promise<void> { | |
| const deadline = Date.now() + timeoutMs | |
| while (Date.now() <= deadline) { | |
| const stats = await stat(path).catch((error: NodeJS.ErrnoException) => { | |
| if (error?.code === 'ENOENT') return null | |
| throw error | |
| }) | |
| if (!stats) return | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| } | |
| const exists = await stat(path) | |
| .then(() => true) | |
| .catch((error: NodeJS.ErrnoException) => { | |
| if (error?.code === 'ENOENT') return false | |
| throw error | |
| }) | |
| assert.equal(exists, false) | |
| } |
🤖 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/__tests__/integration-event-bridge.test.ts` around lines 154 - 162,
The helper waitForPathMissing currently swallows all errors from stat (catch(()
=> null)), which treats permission/IO errors as “missing”; change the catch to
only treat ENOENT as missing and rethrow any other errors: in waitForPathMissing
wrap the stat call with a catch that checks err.code === 'ENOENT' then returns
null, otherwise rethrow the error so tests fail on non-ENOENT issues; reference
the waitForPathMissing function and the stat call to locate the change.
| test('confirmed Slack writeback success removes the local draft command file', async () => { | ||
| const workspaceRoot = await mkdtemp(join(tmpdir(), 'pear-writeback-cleanup-')) | ||
| const localRoot = join(workspaceRoot, 'workspace-id', 'slack', 'channels', 'C123ABC', 'messages') | ||
| const localDraftPath = join(localRoot, 'reply-confirmed.json') | ||
| const remoteDraftPath = '/slack/channels/C123ABC/messages/reply-confirmed.json' | ||
| await mkdir(localRoot, { recursive: true }) | ||
| await writeFile(localDraftPath, JSON.stringify({ text: 'confirmed send' })) | ||
|
|
||
| try { | ||
| const harness = makeHarness(['alice']) | ||
| await harness.bridge.reconcile('project-1', [ | ||
| integration({ | ||
| provider: 'slack', | ||
| integrationId: 'slack-1', | ||
| mountPaths: ['/slack/channels/C123ABC/messages'], | ||
| localMountPaths: [localRoot], | ||
| scope: { notifyAgents: ['alice'] } | ||
| }) | ||
| ]) | ||
|
|
||
| await harness.emit({ | ||
| ...changeEvent(remoteDraftPath, 'slack'), | ||
| type: 'writeback.succeeded' | ||
| } as ChangeEvent) | ||
| await waitForPathMissing(localDraftPath) | ||
|
|
||
| assert.deepEqual(harness.sent, []) | ||
| } finally { | ||
| await rm(workspaceRoot, { recursive: true, force: true }) | ||
| } | ||
| }) | ||
|
|
||
| test('Slack writeback draft cleanup waits for confirmed dispatch', async () => { | ||
| const workspaceRoot = await mkdtemp(join(tmpdir(), 'pear-writeback-cleanup-')) | ||
| const localRoot = join(workspaceRoot, 'workspace-id', 'slack', 'channels', 'C123ABC', 'messages') | ||
| const localDraftPath = join(localRoot, 'reply-pending.json') | ||
| const remoteDraftPath = '/slack/channels/C123ABC/messages/reply-pending.json' | ||
| await mkdir(localRoot, { recursive: true }) | ||
| await writeFile(localDraftPath, JSON.stringify({ text: 'pending send' })) | ||
|
|
||
| try { | ||
| const harness = makeHarness(['alice']) | ||
| await harness.bridge.reconcile('project-1', [ | ||
| integration({ | ||
| provider: 'slack', | ||
| integrationId: 'slack-1', | ||
| mountPaths: ['/slack/channels/C123ABC/messages'], | ||
| localMountPaths: [localRoot], | ||
| scope: { notifyAgents: ['alice'] } | ||
| }) | ||
| ]) | ||
|
|
||
| await harness.emit(changeEvent(remoteDraftPath, 'slack')) | ||
| await waitForDispatcherTick() | ||
|
|
||
| assert.equal(await stat(localDraftPath).then(() => true).catch(() => false), true) | ||
| assert.deepEqual(harness.sent, []) | ||
| } finally { | ||
| await rm(workspaceRoot, { recursive: true, force: true }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add duplicate/replay coverage for confirmed writeback cleanup idempotency.
These tests validate confirmed and pending flows, but they don’t assert behavior when writeback.succeeded is replayed for the same draft (common on event streams). Add a second identical confirmed event and verify no error/no side effects after the first deletion.
As per coding guidelines, **/*.test.{js,ts,tsx} changes touching event streaming or integration notifications should include duplicate/replay cases, not only happy-path flow.
🤖 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/__tests__/integration-event-bridge.test.ts` around lines 2117 -
2177, Add a duplicate/replay check to the "confirmed Slack writeback success
removes the local draft command file" test: after the first harness.emit of the
writeback.succeeded ChangeEvent (remoteDraftPath) and waitForPathMissing, emit
the identical writeback.succeeded event again via harness.emit (same ChangeEvent
payload) and assert the local draft still does not exist (waitForPathMissing or
stat check) and harness.sent remains [] to ensure idempotent cleanup; update
assertions around localDraftPath, remoteDraftPath, harness.emit and
waitForPathMissing to reflect the replayed event case.
Source: Coding guidelines
|
Implemented fixes for the validated Gemini findings. Changed
Added regression coverage in Validation run:
GitHub status check at the end still showed Addressed comments
|
|
✅ pr-reviewer applied fixes — committed and pushed Implemented and verified fixes for the PR review findings. Changes made:
Validation run:
Addressed comments
|
1 participant
Summary
writeback.succeededevents for bounded command rootsVerification
npm testnpx vitest run src/main/integrations.test.tsOps cleanup
.integrations/slack/channels/C0B8ZL2L9GC__pear-pty-investigation/messages.reply-*.json/notice-*.json/announce-*.jsoncount is now 0.