fix: parallelize thread PR fetches and increase Slack API timeout#335
Merged
sweetmantech merged 5 commits intotestfrom Mar 24, 2026
Merged
fix: parallelize thread PR fetches and increase Slack API timeout#335sweetmantech merged 5 commits intotestfrom
sweetmantech merged 5 commits intotestfrom
Conversation
Sequential thread fetches caused timeouts with 30+ tags. Now fetches in parallel batches of 10 and increases per-request timeout from 10s to 30s. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request refactors the Slack mention extraction pipeline by introducing a batching architecture: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant fetchSlackMentions
participant Slack API
participant fetchAllThreadPullRequests
participant buildSlackTags
participant Cache as User Cache
Client->>fetchSlackMentions: Fetch Slack mentions (period)
fetchSlackMentions->>Slack API: Search for user mentions in period
Slack API-->>fetchSlackMentions: Raw mention records
fetchSlackMentions->>fetchAllThreadPullRequests: Batch fetch PRs from threads
loop Parallel batches (10 at a time)
fetchAllThreadPullRequests->>Slack API: Get thread PRs (channelId, ts)
Slack API-->>fetchAllThreadPullRequests: PR URLs
end
fetchAllThreadPullRequests-->>fetchSlackMentions: All PR results (string[][])
fetchSlackMentions->>Cache: Lookup user details
Cache-->>fetchSlackMentions: Names & avatars
fetchSlackMentions->>buildSlackTags: Build tags (mentions, userCache, PRs)
buildSlackTags->>buildSlackTags: Enrich mentions + compute timestamps
buildSlackTags->>buildSlackTags: Sort by timestamp (descending)
buildSlackTags-->>fetchSlackMentions: SlackTag[]
fetchSlackMentions-->>Client: SlackTag[] (enriched, sorted)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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 |
Collect mentions in phase 1, fetch thread PRs in parallel in phase 2, build final SlackTag array in phase 3. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sweetmantech
commented
Mar 24, 2026
lib/slack/slackGet.ts
Outdated
| const res = await fetch(url.toString(), { | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| signal: AbortSignal.timeout(10_000), | ||
| signal: AbortSignal.timeout(30_000), |
Contributor
Author
There was a problem hiding this comment.
Remove this signal altogether.
Comment on lines
+100
to
+117
| // Fetch thread PRs in parallel (batches of 10 to avoid rate limits) | ||
| const BATCH_SIZE = 10; | ||
| for (let i = 0; i < tags.length; i += BATCH_SIZE) { | ||
| const batch = tags.slice(i, i + BATCH_SIZE); | ||
| const results = await Promise.all( | ||
| batch.map(tag => | ||
| fetchThreadPullRequests(token, tag.channel_id, (tag as { _threadTs: string })._threadTs), | ||
| ), | ||
| ); | ||
| for (let j = 0; j < batch.length; j++) { | ||
| batch[j].pull_requests = results[j]; | ||
| } | ||
| } | ||
|
|
||
| // Clean up internal field and sort | ||
| for (const tag of tags) { | ||
| delete (tag as Record<string, unknown>)._threadTs; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Open closed principle
- move new code to a new lib file.
Comment on lines
+101
to
+127
| const BATCH_SIZE = 10; | ||
| const allPullRequests: string[][] = new Array(mentions.length).fill([]); | ||
|
|
||
| for (let i = 0; i < mentions.length; i += BATCH_SIZE) { | ||
| const batch = mentions.slice(i, i + BATCH_SIZE); | ||
| const results = await Promise.all( | ||
| batch.map(m => fetchThreadPullRequests(token, m.channelId, m.ts)), | ||
| ); | ||
| for (let j = 0; j < batch.length; j++) { | ||
| allPullRequests[i + j] = results[j]; | ||
| } | ||
| } | ||
|
|
||
| // Phase 3: Build final tags | ||
| const tags: SlackTag[] = mentions.map((m, i) => { | ||
| const { name, avatar } = userCache[m.userId]; | ||
| return { | ||
| user_id: m.userId, | ||
| user_name: name, | ||
| user_avatar: avatar, | ||
| prompt: m.prompt, | ||
| timestamp: new Date(parseFloat(m.ts) * 1000).toISOString(), | ||
| channel_id: m.channelId, | ||
| channel_name: m.channelName, | ||
| pull_requests: allPullRequests[i], | ||
| }; | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
OCP - new lib for new code.
- Remove signal timeout from slackGet - Extract parallel batch fetching to fetchAllThreadPullRequests.ts - Extract tag building to buildSlackTags.ts - fetchSlackMentions now only collects raw mentions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
Endpoint Testing — Preview DeploymentPreview URL: Results
Analysis
RecommendationThe 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
lib/admins/slack/fetchSlackMentions.ts (2)
89-89: Static analysis: ReDoS warning on dynamic regex.The regex
new RegExp(\<@${botUserId}>\s*`, "g")uses variable input. WhilebotUserIdis controlled (sourced from Slack'sauth.testAPI and follows the formatU[A-Z0-9]+`), static analyzers flag this pattern.The risk here is low since
botUserIdcontains only alphanumeric characters. If you want to silence the warning and be defensive, escape the ID.🛡️ Optional: escape botUserId for safety
+function escapeRegex(str: string): string { + return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + // In the loop: -prompt: (msg.text ?? "").replace(new RegExp(`<@${botUserId}>\\s*`, "g"), "").trim(), +prompt: (msg.text ?? "").replace(new RegExp(`<@${escapeRegex(botUserId)}>\\s*`, "g"), "").trim(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/admins/slack/fetchSlackMentions.ts` at line 89, The dynamic RegExp construction in the prompt assignment (the expression using new RegExp(`<@${botUserId}>\\s*`, "g")) triggers a ReDoS warning; update the code in fetchSlackMentions.ts to defensively escape botUserId before building the RegExp (or avoid RegExp by using a safe string-based replace) so only alphanumeric characters are treated literally; ensure you reference the same expression (the prompt: ... replace/new RegExp usage) and replace it with an escaped-pattern or non-RegExp approach to silence the static analyzer.
34-40: DuplicateRawMentioninterface.This interface is also defined in
buildSlackTags.ts. Extract to a shared types file to comply with DRY.As per coding guidelines: "Extract shared logic into reusable utilities following Don't Repeat Yourself (DRY) principle".
♻️ Proposed extraction
Create
lib/admins/slack/types.ts:export interface RawMention { userId: string; prompt: string; ts: string; channelId: string; channelName: string; }Then import in both files:
import type { RawMention } from "./types";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/admins/slack/fetchSlackMentions.ts` around lines 34 - 40, Duplicate RawMention interface is defined in two places; extract it into a single exported type in a new module (e.g., lib/admins/slack/types.ts) and replace the local definitions with an import: export interface RawMention { userId: string; prompt: string; ts: string; channelId: string; channelName: string; } then in the files that used the duplicate (the modules defining/building Slack tags and fetchSlackMentions) remove the local RawMention declaration and add import type { RawMention } from "./types"; so both modules reference the single shared RawMention type.lib/admins/slack/buildSlackTags.ts (3)
30-30: Assumption of index alignment betweenmentionsandpullRequests.The function relies on
mentionsandpullRequestshaving matching lengths and order. While the caller (fetchSlackMentions) currently ensures this, adding a guard or assertion would make the contract explicit and catch misuse early.🛡️ Optional defensive assertion
export function buildSlackTags( mentions: RawMention[], userCache: Record<string, { name: string; avatar: string | null }>, pullRequests: string[][], ): SlackTag[] { + if (mentions.length !== pullRequests.length) { + throw new Error("mentions and pullRequests arrays must have matching lengths"); + } const tags: SlackTag[] = mentions.map((m, i) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/admins/slack/buildSlackTags.ts` at line 30, The loop in buildSlackTags assumes mentions and pullRequests are aligned; add an explicit guard at the start of buildSlackTags: check that mentions.length === pullRequests.length (or throw an Error/return early with a descriptive log via processLogger) so mismatched input is caught immediately; reference the mentions and pullRequests parameters and the loop that uses pull_requests: pullRequests[i] to locate where to add the assertion/guard.
3-9: Consider extractingRawMentioninterface to a shared types file.This interface is duplicated in
fetchSlackMentions.ts(lines 34-40). To adhere to the DRY principle, extract it to a shared location (e.g.,lib/admins/slack/types.ts) and import it in both files.As per coding guidelines: "Extract shared logic into reusable utilities following Don't Repeat Yourself (DRY) principle".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/admins/slack/buildSlackTags.ts` around lines 3 - 9, The RawMention interface is duplicated; extract it into a shared types module (e.g., create a new types file exporting RawMention) and replace the inline declarations in buildSlackTags.ts and fetchSlackMentions.ts with an import of that shared type. Ensure the exported name is RawMention, update both files to import { RawMention } from the new module, and run TypeScript type checks to confirm no other references need adjustment.
34-34: Sorting can be simplified using localeCompare for ISO strings.Since timestamps are ISO 8601 strings, they sort lexicographically. This avoids redundant
Dateparsing.♻️ Simpler sort
- tags.sort((a, b) => new Date(b.timestamp).getTime() - new Date(a.timestamp).getTime()); + tags.sort((a, b) => b.timestamp.localeCompare(a.timestamp));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/admins/slack/buildSlackTags.ts` at line 34, The sort comparator in buildSlackTags.ts currently parses Dates on every comparison (tags.sort(...)) even though tag.timestamp values are ISO 8601 strings; change the comparator to use string comparison via localeCompare on the timestamp property to perform a lexicographic descending sort (compare b.timestamp to a.timestamp) instead of creating new Date objects.lib/admins/slack/fetchAllThreadPullRequests.ts (1)
19-19: Consider makingBATCH_SIZEconfigurable or documenting the choice.The batch size of 10 is a reasonable default, but documenting why this value was chosen (e.g., Slack rate limits, serverless timeout constraints) would help future maintainers. Alternatively, expose it as an optional parameter for flexibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/admins/slack/fetchAllThreadPullRequests.ts` at line 19, The constant BATCH_SIZE (currently set to 10) in fetchAllThreadPullRequests should be made configurable or documented: either expose it as an optional parameter on the fetchAllThreadPullRequests function (or accept an options object) so callers can override the batch size, or add a clear comment above the BATCH_SIZE declaration explaining why 10 was chosen (e.g., Slack rate limits, serverless timeout, memory). Update any call sites to pass the new parameter where needed and include a short note in surrounding docs/comments describing the rationale and recommended limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/admins/slack/buildSlackTags.ts`:
- Around line 20-21: The destructuring in the mentions.map callback (const {
name, avatar } = userCache[m.userId]) can throw if userCache[m.userId] is
undefined; update the mentions.map in buildSlackTags to defensively handle
missing cache entries by checking for existence of userCache[m.userId] (or using
a default/fallback object) before destructuring and provide sensible fallbacks
for name and avatar when absent so SlackTag creation never throws.
In `@lib/admins/slack/fetchAllThreadPullRequests.ts`:
- Line 20: The results array is initialized with a single shared empty array via
new Array(threads.length).fill([]), which causes all slots to reference the same
array; change the initialization in fetchAllThreadPullRequests (the results
variable) to create distinct sub-arrays for each thread — e.g., use Array.from({
length: threads.length }, () => []) or map over threads to produce a new [] per
index, or simply pre-size results and assign into results[i] in the loop without
fill — ensuring each results[i] is an independent array.
- Around line 22-30: The current batch logic uses Promise.all which aborts the
whole batch if any fetchThreadPullRequests rejects; change it to use
Promise.allSettled (or wrap each fetchThreadPullRequests call with a try/catch)
so individual failures don't reject the batch — for any rejected promise return
an empty array (or a consistent failure placeholder) and still assign results[i
+ j] = batchResult for each item; update the block that constructs batchResults
(the map over batch calling fetchThreadPullRequests) and the subsequent loop
that writes into results to handle settled results (checking status ===
"fulfilled" vs "rejected" and using value or fallback).
---
Nitpick comments:
In `@lib/admins/slack/buildSlackTags.ts`:
- Line 30: The loop in buildSlackTags assumes mentions and pullRequests are
aligned; add an explicit guard at the start of buildSlackTags: check that
mentions.length === pullRequests.length (or throw an Error/return early with a
descriptive log via processLogger) so mismatched input is caught immediately;
reference the mentions and pullRequests parameters and the loop that uses
pull_requests: pullRequests[i] to locate where to add the assertion/guard.
- Around line 3-9: The RawMention interface is duplicated; extract it into a
shared types module (e.g., create a new types file exporting RawMention) and
replace the inline declarations in buildSlackTags.ts and fetchSlackMentions.ts
with an import of that shared type. Ensure the exported name is RawMention,
update both files to import { RawMention } from the new module, and run
TypeScript type checks to confirm no other references need adjustment.
- Line 34: The sort comparator in buildSlackTags.ts currently parses Dates on
every comparison (tags.sort(...)) even though tag.timestamp values are ISO 8601
strings; change the comparator to use string comparison via localeCompare on the
timestamp property to perform a lexicographic descending sort (compare
b.timestamp to a.timestamp) instead of creating new Date objects.
In `@lib/admins/slack/fetchAllThreadPullRequests.ts`:
- Line 19: The constant BATCH_SIZE (currently set to 10) in
fetchAllThreadPullRequests should be made configurable or documented: either
expose it as an optional parameter on the fetchAllThreadPullRequests function
(or accept an options object) so callers can override the batch size, or add a
clear comment above the BATCH_SIZE declaration explaining why 10 was chosen
(e.g., Slack rate limits, serverless timeout, memory). Update any call sites to
pass the new parameter where needed and include a short note in surrounding
docs/comments describing the rationale and recommended limits.
In `@lib/admins/slack/fetchSlackMentions.ts`:
- Line 89: The dynamic RegExp construction in the prompt assignment (the
expression using new RegExp(`<@${botUserId}>\\s*`, "g")) triggers a ReDoS
warning; update the code in fetchSlackMentions.ts to defensively escape
botUserId before building the RegExp (or avoid RegExp by using a safe
string-based replace) so only alphanumeric characters are treated literally;
ensure you reference the same expression (the prompt: ... replace/new RegExp
usage) and replace it with an escaped-pattern or non-RegExp approach to silence
the static analyzer.
- Around line 34-40: Duplicate RawMention interface is defined in two places;
extract it into a single exported type in a new module (e.g.,
lib/admins/slack/types.ts) and replace the local definitions with an import:
export interface RawMention { userId: string; prompt: string; ts: string;
channelId: string; channelName: string; } then in the files that used the
duplicate (the modules defining/building Slack tags and fetchSlackMentions)
remove the local RawMention declaration and add import type { RawMention } from
"./types"; so both modules reference the single shared RawMention type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7cce05d3-ee9b-4b1d-b5c9-d5ebc4724463
📒 Files selected for processing (4)
lib/admins/slack/buildSlackTags.tslib/admins/slack/fetchAllThreadPullRequests.tslib/admins/slack/fetchSlackMentions.tslib/slack/slackGet.ts
💤 Files with no reviewable changes (1)
- lib/slack/slackGet.ts
Comment on lines
+20
to
+21
| const tags: SlackTag[] = mentions.map((m, i) => { | ||
| const { name, avatar } = userCache[m.userId]; |
There was a problem hiding this comment.
Potential runtime error if user is missing from cache.
The destructuring const { name, avatar } = userCache[m.userId] will throw a TypeError if m.userId is not a key in userCache. This could occur if upstream logic fails to populate the cache for a user.
🛡️ Proposed defensive fix
const tags: SlackTag[] = mentions.map((m, i) => {
- const { name, avatar } = userCache[m.userId];
+ const cached = userCache[m.userId] ?? { name: "Unknown", avatar: null };
+ const { name, avatar } = cached;
return {📝 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
| const tags: SlackTag[] = mentions.map((m, i) => { | |
| const { name, avatar } = userCache[m.userId]; | |
| const tags: SlackTag[] = mentions.map((m, i) => { | |
| const cached = userCache[m.userId] ?? { name: "Unknown", avatar: null }; | |
| const { name, avatar } = cached; | |
| return { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/admins/slack/buildSlackTags.ts` around lines 20 - 21, The destructuring
in the mentions.map callback (const { name, avatar } = userCache[m.userId]) can
throw if userCache[m.userId] is undefined; update the mentions.map in
buildSlackTags to defensively handle missing cache entries by checking for
existence of userCache[m.userId] (or using a default/fallback object) before
destructuring and provide sensible fallbacks for name and avatar when absent so
SlackTag creation never throws.
- slackGet retries once on 429 using Retry-After header - Reduce parallel batch size from 10 to 5 - Add 1.1s delay between batches to respect Slack rate limits - Add tests for 429 retry behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ilience - Array.from creates unique arrays per slot (fill([]) shared one reference) - Promise.allSettled prevents one failed thread from crashing the batch Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 24, 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
1 participant
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.
Summary
Problem
GET /api/admins/coding/slackreturns 500 forperiod=weeklyand longer — each tag sequentially callsconversations.replies, timing out the serverless function.Test plan
period=weeklyandperiod=allreturn 200 on preview deploymentperiod=dailystill works🤖 Generated with Claude Code
Summary by CodeRabbit
Performance Improvements
Bug Fixes