fix(work-apps): Slack api pagination#1994
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 973e189 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
client.ts:234Missing limit override causescheckUserIsChannelMemberto fail for channels with 200+ members
💭 Consider (4) 💭
💭 1) client.ts:122-132 Silent ok: false handling in extractItems
Issue: The extractItems callback silently returns an empty array when result.ok is false, masking mid-pagination API failures without logging.
Why: If Slack API returns { ok: false, error: 'ratelimited' } on a later page, the error is silently swallowed and items from that page are dropped.
Fix: Consider adding a warning log when ok: false is encountered:
extractItems: (result) => {
if (!result.ok) {
logger.warn({ error: result.error }, 'Slack API returned ok: false during pagination');
return [];
}
return result.channels ? result.channels.map(...) : [];
}Refs: client.ts:122-132
💭 2) client.test.ts Missing test for mid-pagination errors
Issue: No test verifies behavior when an API call fails after the first page succeeds. The current tests only cover errors on the first call.
Why: Edge case that could surface unexpected behavior if Slack rate-limits mid-pagination.
Fix: Consider adding: it('should return empty array when error occurs mid-pagination') — mock first page success with next_cursor, second page rejects.
Refs: client.test.ts:383-405
💭 3) client.test.ts:442-471 Test parameter verification could be more complete
Issue: The checkUserIsChannelMember pagination tests use expect.objectContaining({ cursor: ... }) but don't verify channel and limit parameters are passed correctly.
Why: If someone refactors and accidentally removes channel: channelId, no test would catch it.
Fix: Consider using full parameter assertions: expect(mockClient.conversations.members).toHaveBeenNthCalledWith(1, { channel: 'C123', limit: 200, cursor: undefined })
Refs: client.test.ts:464-471
💭 4) .changeset/ Missing changeset
Issue: The changeset-bot flagged this PR has no changeset.
Why: This is a bug fix that changes behavior (pagination + default limit change from 20 to 200), which should be documented for consumers.
Fix: Run pnpm bump patch --pkg agents-work-apps "Fix Slack API pagination for channels and membership checks"
Refs: changeset-bot comment
🚫 REQUEST CHANGES
Summary: The pagination refactoring introduces a critical regression in checkUserIsChannelMember — the function now stops after 200 members instead of checking all members, causing false authorization failures for legitimate channel members in large Slack channels. This is a one-line fix (pass limit: Infinity to paginateSlack). Once fixed, the PR's pagination abstraction is a solid improvement. The Consider items are optional enhancements for robustness.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
client.ts:20-36 |
paginateSlack helper follows existing patterns | INFO — validation of good pattern, not actionable |
client.ts:112 |
Default limit change from 20 to 200 | INFO — reasonable optimization aligned with API page size |
client.ts:224-236 |
Early-exit optimization lost | Subsumed by Critical finding — fixing limit makes this less relevant |
client.ts:20-36 |
Partial results lost on mid-pagination errors | Pre-existing behavior, not a regression introduced by this PR |
client.test.ts |
Missing test for 200+ member scenario | Merged into Critical finding as supporting evidence |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-errors |
3 | 0 | 2 | 0 | 0 | 0 | 1 |
pr-review-tests |
3 | 0 | 2 | 0 | 0 | 0 | 1 |
pr-review-consistency |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 10 | 0 | 4 | 0 | 1 | 0 | 5 |
| }), | ||
| extractItems: (result) => (result.ok && result.members ? result.members : []), | ||
| getNextCursor: (result) => result.response_metadata?.next_cursor || undefined, | ||
| }); |
There was a problem hiding this comment.
🔴 CRITICAL: Missing limit override causes authorization failures in large channels
Issue: The refactored checkUserIsChannelMember does not pass a limit parameter to paginateSlack, causing it to default to 200. The original implementation iterated through ALL members without a limit. For Slack channels with more than 200 members, this will incorrectly return false for users who are legitimate members but appear beyond the first 200 results.
Why: This function is used for authorization in requireChannelMemberOrAdmin middleware (permissions.ts:217). A false false means legitimate channel members get denied access with "You can only configure channels you are a member of" — a security-impacting false negative.
Fix:
| }); | |
| const members = await paginateSlack({ | |
| fetchPage: (cursor) => | |
| client.conversations.members({ | |
| channel: channelId, | |
| limit: 200, | |
| cursor, | |
| }), | |
| extractItems: (result) => (result.ok && result.members ? result.members : []), | |
| getNextCursor: (result) => result.response_metadata?.next_cursor || undefined, | |
| limit: Infinity, | |
| }); |
Refs:
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
client.ts:36Mid-pagination exceptions discard previously collected items
💭 Consider (3) 💭
💭 1) client.test.ts:461-575 Missing test for checkUserIsChannelMember mid-pagination error
Issue: The delta added warning logs for ok: false responses during members pagination (client.ts:246-251), but there's no test verifying this code path.
Why: The getSlackChannels tests include mid-pagination error tests (lines 388-434), but checkUserIsChannelMember tests only cover ok: false on the first call. Without a test, regressions could change this behavior silently.
Fix: Add a test similar to the getSlackChannels mid-pagination test: mock first page returning members, second page returning ok: false, verify the function returns false.
Refs: client.test.ts:388-433 — getSlackChannels mid-pagination tests
💭 2) client.ts:128-133 Warning log lacks pagination context
Issue: The ok: false warning log in getSlackChannels only includes { error: result.error } but not which page failed or how many channels were already collected.
Why: When debugging a partial channel list, operators cannot determine if the failure happened on page 1 (no data lost) or page 10 (significant data affected).
Fix: Consider including pagination context: { error: result.error, itemsCollectedSoFar: items.length, cursor } — though this may require refactoring to pass accumulator state to the callback.
Refs: client.ts:128-133
💭 3) client.ts:246-251 Warning log lacks channel/user context
Issue: The ok: false warning log in checkUserIsChannelMember only includes { error: result.error } but not which channel or user was being checked.
Why: When debugging authorization failures, operators cannot correlate the warning to a specific membership check.
Fix: Since the extractItems closure has access to channelId and userId from the outer scope, include them: { error: result.error, channelId, userId }.
Refs: client.ts:246-251
💡 APPROVE WITH SUGGESTIONS
Summary: The critical issue from the prior review has been properly addressed — checkUserIsChannelMember() now correctly iterates through ALL channel members by making the limit parameter optional in paginateSlack(). The author also addressed the Consider items by adding error logging, improving test assertions, and adding a changeset. 🎉
The one Major finding (mid-pagination exception handling) is worth considering but doesn't block merging — the current "fail-closed" behavior is defensive and consistent with how the outer try-catch was previously designed. The Consider items are enhancements for observability and test coverage.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
client.test.ts |
Tests don't verify warning log is emitted | Too minor (LOW confidence) — nice-to-have for completeness but not actionable |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
2 | 0 | 1 | 0 | 0 | 0 | 1 |
pr-review-errors |
3 | 0 | 2 | 0 | 1 | 0 | 0 |
| Total | 5 | 0 | 3 | 0 | 1 | 0 | 1 |
Note: pr-review-standards confirmed the Critical issue from prior review was properly addressed — no new code quality issues found.
| } while (cursor && (limit === undefined || items.length < limit)); | ||
|
|
||
| return limit !== undefined ? items.slice(0, limit) : items; | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Mid-pagination exceptions discard previously collected items
Issue: When a thrown exception occurs after successfully fetching earlier pages, the outer try-catch in getSlackChannels() catches it and returns [] instead of returning the partial results already collected.
Why: For a workspace with 500 channels where an error occurs fetching page 3, users get an empty channel list instead of the 200+ channels already fetched. This is inconsistent with the ok: false handling which correctly preserves partial results — creating confusing behavior where soft failures preserve data but hard failures lose it.
Fix: Consider moving error handling into the pagination helper to preserve accumulated items on fetch failures:
do {
let response: TResponse;
try {
response = await fetchPage(cursor);
} catch (error) {
// Return what we have on fetch failure
logger.warn({ error, itemsCollected: items.length }, 'Pagination fetch failed, returning partial results');
break;
}
items.push(...extractItems(response));
cursor = getNextCursor(response);
} while (cursor && (limit === undefined || items.length < limit));Alternatively, if the current "fail-closed" behavior (return empty on error) is intentional, consider documenting this as an explicit design choice for consumers to understand.
Refs:
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
🧹 While You're Here (1) 🧹
🧹 1) workspaces.ts:767 Unhandled error propagation in bulk channel operation
Issue: The getSlackChannels() call at line 767 in the bulk channel update route is not wrapped in try-catch. This PR's delta changed getSlackChannels() to propagate errors instead of swallowing them, which exposes this call site to unhandled exceptions.
Why: This was always a latent issue — the call site was written expecting the "swallow errors and return empty" behavior. Now that errors propagate, Slack API failures during bulk operations will result in unstructured 500 errors instead of the expected { success, updated, failed, errors } response format. Compare with the single-channel list route (lines 526-571) which properly catches and returns structured error responses.
Fix: Wrap the getSlackChannels() call in try-catch consistent with the pattern at line 526-571:
try {
const channels = await getSlackChannels(slackClient, 500);
// ... rest of handler
} catch (error) {
logger.error({ error, teamId }, 'Failed to fetch channels for bulk operation');
return c.json({ error: 'Failed to fetch channels' }, 500);
}Refs: workspaces.ts:526-571 — existing error handling pattern
🕐 Pending Recommendations (1)
- 💭
client.test.ts:549-571Missing tests forcheckUserIsChannelMembermid-pagination errors (bothok: falseand thrown errors)
✅ APPROVE
Summary: The delta (5ab6fefc let api routes handle errors) addresses the Major finding from the prior review about mid-pagination exceptions discarding partial results — errors now propagate to API route handlers, which is the correct pattern for these utility functions. The "While You're Here" item about workspaces.ts:767 is a pre-existing call site that was exposed by this behavioral change; it's worth addressing but doesn't block this PR. The test coverage improvements suggested in the prior review remain valid considerations. Nice work cleaning up the error handling! 🎉
Discarded (0)
| Location | Issue | Reason Discarded |
|---|---|---|
| — | — | No findings discarded |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
2 | 0 | 0 | 0 | 0 | 1 | 1 |
pr-review-errors |
1 | 0 | 0 | 1 | 0 | 0 | 0 |
| Total | 3 | 0 | 0 | 1 | 0 | 1 | 1 |
Note: The pr-review-tests finding about mid-pagination tests was deduplicated against the prior review's Consider item #1. The second test finding was merged into the pending recommendation.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
The delta (973e1895 handle channel fetch error) adds proper error handling around the getSlackChannels() call at line 767-773 in workspaces.ts. This directly addresses the "While You're Here" item from the prior review.
Delta Assessment
The 7-line change:
let channels: Awaited<ReturnType<typeof getSlackChannels>> = [];
try {
channels = await getSlackChannels(slackClient, 500);
} catch (error) {
logger.error({ error, teamId }, 'Failed to fetch channels for bulk operation');
return c.json({ error: 'Failed to fetch channels' }, 500);
}Follows the exact pattern used elsewhere in the same file (lines 568-571 for the single-channel list route). Error is logged with context (teamId), and a structured 500 response is returned. ✅
Prior Feedback Status
| Item | Status |
|---|---|
🔴 Critical: Missing limit override in checkUserIsChannelMember |
✅ Resolved in 265756d8 |
| 🟠 Major: Mid-pagination exceptions discard partial results | ✅ Addressed — errors propagate to route handlers, call sites handle them |
🧹 While You're Here: Unhandled error at workspaces.ts:767 |
✅ Resolved by this delta |
🕐 Pending Recommendations (1)
- 💭
client.test.tsMissing tests forcheckUserIsChannelMembermid-pagination errors — optional but would improve edge case coverage
✅ APPROVE
Summary: The delta cleanly addresses the "While You're Here" item from the prior review. All call sites for getSlackChannels() now have proper error handling. The pagination refactoring is solid, with good test coverage for the happy paths. The optional test enhancement for mid-pagination edge cases can be addressed in a follow-up if desired. Nice work iterating on the feedback! 🎉
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: No sub-reviewers dispatched for this 7-line delta. The change was trivial error handling that exactly matches existing patterns in the file.
This reverts commit 7417653.
* fix channel api pagination * update default pagination limit * let api routes handle errors * handle channel fetch error
2 participants
No description provided.