Fix Slack integration event context preview#130
Conversation
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Warning Review limit reached
More reviews will be available in 54 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Slack-targeted read retry/backoff, sparse relayfile pointer detection to avoid empty previews, refactors Slack message assembly and dedupe/injection logic, and extends tests and harness to simulate and validate transient read failures and duplicate-suppression scenarios. ChangesSlack Context Retry & Formatting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 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 retry logic for reading Slack event context previews, filters out sparse relayfile pointer data from context previews, adjusts the formatting of Slack integration event messages, and adds corresponding integration tests. The review feedback suggests two improvements: first, aborting the retry loop early on unauthorized or forbidden errors (401/403) to prevent blocking the sequential event queue; second, using bracket notation instead of dot notation when accessing properties on Record<string, unknown> to ensure TypeScript compilation safety under strict configurations.
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.
| for (const [index, delayMs] of readDelays.entries()) { | ||
| if (delayMs > 0) await delay(delayMs) | ||
| try { | ||
| return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path)) | ||
| } catch (error) { | ||
| readFileError = error | ||
| if (index === readDelays.length - 1) break | ||
| } | ||
| } |
There was a problem hiding this comment.
If client.readFile fails with an unauthorized or forbidden error (401/403), retrying is guaranteed to fail and will introduce unnecessary delays (up to 1.25 seconds total). Since the event dispatcher processes and awaits event deliveries sequentially, this delay will block the entire event queue for the project. Aborting the retry loop early on unauthorized errors prevents this performance bottleneck.
| for (const [index, delayMs] of readDelays.entries()) { | |
| if (delayMs > 0) await delay(delayMs) | |
| try { | |
| return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path)) | |
| } catch (error) { | |
| readFileError = error | |
| if (index === readDelays.length - 1) break | |
| } | |
| } | |
| for (const [index, delayMs] of readDelays.entries()) { | |
| if (delayMs > 0) await delay(delayMs) | |
| try { | |
| return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path)) | |
| } catch (error) { | |
| readFileError = error | |
| if (isUnauthorizedError(error) || index === readDelays.length - 1) break | |
| } | |
| } |
| function isSparseRelayfilePointerData(path: string, data: unknown): boolean { | ||
| if (!isRecord(data)) return false | ||
| const keys = Object.keys(data).sort() | ||
| if (keys.length === 1 && keys[0] === 'path') return data.path === path | ||
| if (keys.length === 2 && keys[0] === 'deleted' && keys[1] === 'path') { | ||
| return data.path === path && typeof data.deleted === 'boolean' | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Accessing properties via dot notation on Record<string, unknown> (returned by isRecord) can cause TypeScript compilation errors under strict compiler configurations (e.g., with noPropertyAccessFromIndexSignature or strict type checking). Using bracket notation is safer and more idiomatic for index-signature types.
| function isSparseRelayfilePointerData(path: string, data: unknown): boolean { | |
| if (!isRecord(data)) return false | |
| const keys = Object.keys(data).sort() | |
| if (keys.length === 1 && keys[0] === 'path') return data.path === path | |
| if (keys.length === 2 && keys[0] === 'deleted' && keys[1] === 'path') { | |
| return data.path === path && typeof data.deleted === 'boolean' | |
| } | |
| return false | |
| } | |
| function isSparseRelayfilePointerData(path: string, data: unknown): boolean { | |
| if (!isRecord(data)) return false | |
| const keys = Object.keys(data).sort() | |
| if (keys.length === 1 && keys[0] === 'path') return data['path'] === path | |
| if (keys.length === 2 && keys[0] === 'deleted' && keys[1] === 'path') { | |
| return data['path'] === path && typeof data['deleted'] === 'boolean' | |
| } | |
| return false | |
| } |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
![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
🤖 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/integration-event-bridge.ts`:
- Around line 2250-2262: The read loop in readEventContextPreview keeps retrying
every error from client.readFile (called via
getWorkspaceHandle().client().readFile) including permanent auth/permission
failures; change the loop in readEventContextPreview so that after catching an
error you detect permanent/non-transient failures (e.g., auth, 403/401,
permission-denied, token-expired semantics or use an isTransient/error.code
check) and immediately break/return to fall back to expand('full') instead of
waiting for subsequent delays from EVENT_CONTEXT_READ_RETRY_DELAYS_MS; only
await delay and retry when the error is identified as transient.
🪄 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: 28bc8804-91d3-47dc-8ba0-0aacc12544a5
📒 Files selected for processing (2)
src/main/__tests__/integration-event-bridge.test.tssrc/main/integration-event-bridge.ts
| const readDelays = slackEventContextPath(path) ? [0, ...EVENT_CONTEXT_READ_RETRY_DELAYS_MS] : [0] | ||
| const handle = await this.getWorkspaceHandle() | ||
| const client = handle.client() | ||
| if (typeof client.readFile === 'function') { | ||
| return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path)) | ||
| for (const [index, delayMs] of readDelays.entries()) { | ||
| if (delayMs > 0) await delay(delayMs) | ||
| try { | ||
| return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path)) | ||
| } catch (error) { | ||
| readFileError = error | ||
| if (index === readDelays.length - 1) break | ||
| } | ||
| } |
There was a problem hiding this comment.
Stop retrying permanent Slack preview read failures.
This loop retries every Slack readFile error, including permanent auth/permission failures. Because injectEvent() awaits readEventContextPreview() on the per-project dispatch path, a broken session turns each Slack event into four doomed remote reads and ~1.25s of avoidable head-of-line delay before falling back to expand('full'). Break out immediately for non-transient errors and only delay-retry the transient ones.
Suggested fix
for (const [index, delayMs] of readDelays.entries()) {
if (delayMs > 0) await delay(delayMs)
try {
return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path))
} catch (error) {
readFileError = error
- if (index === readDelays.length - 1) break
+ if (isUnauthorizedError(error) || index === readDelays.length - 1) break
}
}🤖 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/integration-event-bridge.ts` around lines 2250 - 2262, The read loop
in readEventContextPreview keeps retrying every error from client.readFile
(called via getWorkspaceHandle().client().readFile) including permanent
auth/permission failures; change the loop in readEventContextPreview so that
after catching an error you detect permanent/non-transient failures (e.g., auth,
403/401, permission-denied, token-expired semantics or use an
isTransient/error.code check) and immediately break/return to fall back to
expand('full') instead of waiting for subsequent delays from
EVENT_CONTEXT_READ_RETRY_DELAYS_MS; only await delay and retry when the error is
identified as transient.
|
Fixed the PR issues I found:
Local validation passed:
Note: the checked-out |
|
Fixed the PR issues I found:
Local validation passed:
Note: the checked-out |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/integration-event-bridge.ts">
<violation number="1" location="src/main/integration-event-bridge.ts:2584">
P2: Slack dedupe treats a prior sparse preview as a permanent duplicate match, which can suppress real later message updates for the full replay TTL.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/integration-event-bridge.ts">
<violation number="1" location="src/main/integration-event-bridge.ts:2262">
P2: The retry loop does not distinguish between transient and permanent errors. When `readFile` fails with a 401/403/auth/permission error, the loop still performs all remaining retries (up to 4 total attempts with ~1.25s cumulative delay). Since `isUnauthorizedError()` is already defined in this file, use it to break out of the loop immediately on permanent failures:
```typescript
} catch (error) {
readFileError = error
if (index === readDelays.length - 1 || isUnauthorizedError(error)) break
}
```</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path)) | ||
| } catch (error) { | ||
| readFileError = error | ||
| if (index === readDelays.length - 1) break |
There was a problem hiding this comment.
P2: The retry loop does not distinguish between transient and permanent errors. When readFile fails with a 401/403/auth/permission error, the loop still performs all remaining retries (up to 4 total attempts with ~1.25s cumulative delay). Since isUnauthorizedError() is already defined in this file, use it to break out of the loop immediately on permanent failures:
} catch (error) {
readFileError = error
if (index === readDelays.length - 1 || isUnauthorizedError(error)) break
}Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/integration-event-bridge.ts, line 2262:
<comment>The retry loop does not distinguish between transient and permanent errors. When `readFile` fails with a 401/403/auth/permission error, the loop still performs all remaining retries (up to 4 total attempts with ~1.25s cumulative delay). Since `isUnauthorizedError()` is already defined in this file, use it to break out of the loop immediately on permanent failures:
```typescript
} catch (error) {
readFileError = error
if (index === readDelays.length - 1 || isUnauthorizedError(error)) break
}
```</comment>
<file context>
@@ -2233,10 +2249,19 @@ export class IntegrationEventBridge {
+ return eventContextPreviewFromFile(await client.readFile(handle.workspaceId, path))
+ } catch (error) {
+ readFileError = error
+ if (index === readDelays.length - 1) break
+ }
+ }
</file context>
| if (index === readDelays.length - 1) break | |
| if (index === readDelays.length - 1 || isUnauthorizedError(error)) break |
|
Fixed one PR breakage in integration-event-bridge.test.ts: Validated locally:
|
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
✅ pr-reviewer applied fixes — committed and pushed Fixed one PR breakage in integration-event-bridge.test.ts: Validated locally:
|
|
Fixed two validated issues in PR #130:
Verified with:
|
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
✅ pr-reviewer applied fixes — committed and pushed Fixed two validated issues in PR #130:
Verified with:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/integration-event-bridge.ts (1)
2390-2391: 💤 Low valueRedundant
deduperecomputation.
dedupeis recomputed with identical inputs (duplicateKey,fingerprint) sincecontextPreviewno longer affectseventDedupeKeyWithFingerprint. This assignment can be removed.Suggested fix
if (needsSlackContentAwareDedupe) { - dedupe = eventDedupeKeyWithFingerprint(duplicateKey, fingerprint) if (!this.claimSlackLogicalInjection(dedupe.key, contextPreview, dedupe.ttlMs)) {🤖 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/integration-event-bridge.ts` around lines 2390 - 2391, Remove the redundant recomputation of dedupe inside the needsSlackContentAwareDedupe branch: the variable dedupe is being reassigned using eventDedupeKeyWithFingerprint(duplicateKey, fingerprint) with the same inputs already used elsewhere and contextPreview no longer affects that function, so delete the assignment in the if (needsSlackContentAwareDedupe) block (leave any other logic in that block intact) and rely on the previously computed dedupe value; reference: dedupe, eventDedupeKeyWithFingerprint, duplicateKey, fingerprint, needsSlackContentAwareDedupe.
🤖 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.
Nitpick comments:
In `@src/main/integration-event-bridge.ts`:
- Around line 2390-2391: Remove the redundant recomputation of dedupe inside the
needsSlackContentAwareDedupe branch: the variable dedupe is being reassigned
using eventDedupeKeyWithFingerprint(duplicateKey, fingerprint) with the same
inputs already used elsewhere and contextPreview no longer affects that
function, so delete the assignment in the if (needsSlackContentAwareDedupe)
block (leave any other logic in that block intact) and rely on the previously
computed dedupe value; reference: dedupe, eventDedupeKeyWithFingerprint,
duplicateKey, fingerprint, needsSlackContentAwareDedupe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d03e63e8-ff70-405f-80db-2371245b6962
📒 Files selected for processing (2)
src/main/__tests__/integration-event-bridge.test.tssrc/main/integration-event-bridge.ts
Main independently landed the Slack context-preview/dedupe line as #130 with a stronger claim shape: contentHashes as a Set, so late alias copies of ANY previously seen edit stay suppressed (the single-hash variant would re-inject a stale copy of edit N-1 after edit N). Resolution keeps main's Set design and re-applies this branch's blind-claim fix on top: a blind claim learns the hash from the suppressed late content copy, so genuine edits within the TTL still inject. The edit-after-blind regression test is retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary:
Tests: