feat(slack): Enhanced DM management and file operations with API-aligned typings #1376
Conversation
…ty and improved response structures
|
Warning Rate limit exceeded@yuriassuncx has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 30 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (3)
WalkthroughReworks Slack client typings and API wrappers; changes DM send to postMessage with richer response; adds conversation-open loader, file upload (V2 + legacy) actions and files list loader; removes file download/info actions; adds cursor pagination to history loaders; updates manifest and OAuth scopes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant DMAction as sendDm
participant SlackClient
participant SlackAPI as Slack API
App->>DMAction: sendDm({ userId, text })
DMAction->>SlackClient: postMessage(userId, text, { thread_ts?, blocks? })
SlackClient->>SlackAPI: POST /chat.postMessage
SlackAPI-->>SlackClient: { ok, channel, ts, message, warning?, error?, response_metadata? }
SlackClient-->>DMAction: SlackResponse
alt response.ok
DMAction-->>App: SendDmResponse { success: true, channelId, ts, messageData }
else
DMAction-->>App: SendDmResponse { success: false, message: error }
end
note right of SlackAPI: Slack auto-opens DM when posting to a user ID
sequenceDiagram
autonumber
actor App
participant UploadAction as uploadFile / uploadFileV2
participant SlackClient
participant SlackAPI as Slack API
App->>UploadAction: uploadFile({ channels, file, filename, ... })
UploadAction->>SlackClient: uploadFile(...) or uploadFileV2(...)
SlackClient->>SlackAPI: POST /files.upload (multipart) or V2 endpoints
SlackAPI-->>SlackClient: { ok, file|files, warning?, response_metadata?, error? }
SlackClient-->>UploadAction: SlackResponse
alt response.ok
UploadAction-->>App: UploadFileResponse { ok: true, files/file, response_metadata? }
else
UploadAction-->>App: UploadFileResponse { ok: false, error }
end
sequenceDiagram
autonumber
actor App
participant Loader as openConversation
participant SlackClient
participant SlackAPI as Slack API
App->>Loader: openConversation({ userId })
Loader->>SlackClient: openDmChannel(userId)
SlackClient->>SlackAPI: POST /conversations.open
SlackAPI-->>SlackClient: { ok, channel?, no_op?, already_open?, warning?, response_metadata?, error? }
SlackClient-->>Loader: SlackResponse
Loader-->>App: ConversationOpenResponse (mapped fields)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Tagging OptionsShould a new tag be published when this PR is merged?
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
slack/client.ts (1)
290-302: Standardize SlackResponse return shape across methods that advertise it.Several methods declare
SlackResponse<...>but return raw Slack JSON. Wrap results consistently (as done ingetChannelHistory/listDmChannels) to prevent consumer breakage.async joinChannel( channelId: string, ): Promise<SlackResponse<{ channel: SlackChannel }>> { const response = await fetch("https://slack.com/api/conversations.join", { @@ - return response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { channel: result.channel }, + }; } @@ async postReply( @@ - ): Promise< - SlackResponse<{ channel: string; ts: string; message: SlackMessage }> - > { + ): Promise<SlackResponse<{ channel: string; ts: string; message: SlackMessage }>> { @@ - return response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { channel: result.channel, ts: result.ts, message: result.message }, + }; } @@ async addReaction( @@ - ): Promise<SlackResponse<{ channel: string; ts: string }>> { + ): Promise<SlackResponse<{ channel: string; ts: string }>> { @@ - return response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { channel: result.channel, ts: result.ts }, + }; } @@ async getThreadReplies( @@ - ): Promise<SlackResponse<{ messages: SlackMessage[] }>> { + ): Promise<SlackResponse<{ messages: SlackMessage[] }>> { @@ - return response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { messages: result.messages || [] }, + }; } @@ async getUsers( @@ - ): Promise<SlackResponse<{ members: SlackUser[] }>> { + ): Promise<SlackResponse<{ members: SlackUser[] }>> { @@ - return response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { members: result.members || [] }, + }; } @@ async getUserProfile( userId: string, ): Promise<SlackResponse<{ profile: SlackUserProfile }>> { @@ - return response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { profile: result.profile }, + }; } @@ async testAuth(): Promise<SlackResponse<SlackAuthTestResponse>> { @@ - return response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { + url: result.url, + team: result.team, + user: result.user, + team_id: result.team_id, + user_id: result.user_id, + bot_id: result.bot_id, + is_enterprise_install: result.is_enterprise_install, + enterprise_id: result.enterprise_id, + }, + }; }Also applies to: 349-367, 375-391, 444-459, 467-486, 492-506, 511-517
🧹 Nitpick comments (16)
slack/loaders/dms/list.ts (1)
34-37: Normalize empty next_cursor to undefinedAvoid returning an empty string and make consumer checks simpler.
return { channels: dmResponse.data.channels, - next_cursor: dmResponse.data.response_metadata?.next_cursor, + next_cursor: dmResponse.data.response_metadata?.next_cursor || undefined, };slack/loaders/dms/history.ts (1)
48-48: Defensive access for messages pathHarden against clients that don’t wrap in
.data.- return historyResponse.data.messages || []; + return historyResponse.data?.messages ?? historyResponse.messages ?? [];slack/loaders/files/list.ts (1)
49-55: Add runtime validation and sanitize paginationEnsure required input and clamp values before calling the client.
export default async function listUserFiles( props: Props, _req: Request, ctx: AppContext, ): Promise<FilesListResponse> { try { - const response = await ctx.slack.listUserFiles( - props.userId, - props.count || 20, - props.page || 1, - props.types || 'all' - ); + if (!props.userId) { + return { + ok: false, + files: [], + paging: { count: 0, total: 0, page: 1, pages: 0 }, + error: "userId is required", + }; + } + const count = Math.max(1, props.count ?? 20); + const page = Math.max(1, props.page ?? 1); + const types = props.types ?? "all"; + const response = await ctx.slack.listUserFiles( + props.userId, + count, + page, + types, + );slack/actions/files/upload.ts (3)
1-2: Import SlackFile type for reusePrevent type drift by reusing the central file type.
-import type { AppContext } from "../../mod.ts"; +import type { AppContext } from "../../mod.ts"; +import type { SlackFile } from "../../client.ts";
35-71: Reuse SlackFile instead of inlining the shapeReduces maintenance and stays aligned with client typings.
export interface UploadFileResponse { ok: boolean; - file?: { - id: string; - created: number; - timestamp: number; - name: string; - title: string; - mimetype: string; - filetype: string; - pretty_type: string; - user: string; - editable: boolean; - size: number; - mode: string; - is_external: boolean; - external_type: string; - is_public: boolean; - public_url_shared: boolean; - display_as_bot: boolean; - username: string; - url_private: string; - url_private_download: string; - permalink: string; - permalink_public?: string; - channels: string[]; - groups: string[]; - ims: string[]; - comments_count: number; - is_starred?: boolean; - }; + file?: SlackFile; error?: string; warning?: string; response_metadata?: { warnings?: string[]; };
82-90: Validate inputs and accept data URLsFail fast and trim data URL prefixes for base64 strings.
export default async function uploadFile( props: UploadFileProps, _req: Request, ctx: AppContext, ): Promise<UploadFileResponse> { try { + if (!props.channels || !props.filename || !props.file) { + return { ok: false, error: "channels, filename, and file are required" }; + } + const sanitizedFile = + typeof props.file === "string" && props.file.startsWith("data:") + ? (props.file.split(",", 2)[1] ?? "") + : props.file; const response = await ctx.slack.uploadFile({ channels: props.channels, - file: props.file, + file: sanitizedFile, filename: props.filename, title: props.title, initial_comment: props.initial_comment, filetype: props.filetype, });slack/actions/dms/send.ts (5)
20-34: Expose ok alias for consistency with other Slack actionsKeep
successfor compatibility; addokto align withUploadFileResponse/loaders.export interface SendDmResponse { success: boolean; + ok?: boolean; message: string; channelId?: string; ts?: string; messageData?: {
51-55: Set ok=false on failureMinor shape consistency.
if (!messageResponse.ok) { return { - success: false, + success: false, + ok: false, message: `Failed to send DM: ${messageResponse.error || "Unknown error"}`, }; }
58-70: Set ok=true on successKeep both flags for now.
return { - success: true, + success: true, + ok: true, message: "DM sent successfully", channelId: messageResponse.channel, ts: messageResponse.ts,
75-77: Set ok=false in catchMirror the failure shape.
return { - success: false, + success: false, + ok: false, message: `Error sending DM: ${message}`, };
14-18: Type blocks more strictlyPrefer a typed Block array to improve DX; fallback to
Record<string, unknown>[]if Block types aren’t centralized.- blocks?: unknown[]; + blocks?: Record<string, unknown>[];slack/loaders/conversations/open.ts (1)
27-33: Add lightweight input validation for userId.Guard against empty/whitespace-only
userIdto fail fast.export default async function openConversation( props: Props, _req: Request, ctx: AppContext, ): Promise<ConversationOpenResponse> { + if (!props.userId?.trim()) { + return { ok: false, error: "userId is required" }; + } try {slack/client.ts (4)
401-437: Remove nested response_metadata duplication in getChannelHistory.Keep
response_metadataonly at top-level.return { ok: result.ok, error: result.error, response_metadata: result.response_metadata, data: { messages: result.messages || [], has_more: result.has_more, pin_count: result.pin_count, channel_actions_ts: result.channel_actions_ts, channel_actions_count: result.channel_actions_count, warning: result.warning, - response_metadata: result.response_metadata, }, };
599-609: Also remove nested response_metadata in listDmChannels.return { ok: result.ok, error: result.error, response_metadata: result.response_metadata, data: { channels: result.channels || [], - response_metadata: result.response_metadata, }, };
650-709: Improve base64 handling and memory profile in uploadFile; support data URLs.
- Accept
data:*;base64,strings by stripping the prefix.- Build the byte array without an intermediate JS array and set an optional MIME type.
- // Handle file content - if (typeof options.file === "string") { - // Assume base64 string, convert to Blob - const byteCharacters = atob(options.file); - const byteNumbers = new Array(byteCharacters.length); - for (let i = 0; i < byteCharacters.length; i++) { - byteNumbers[i] = byteCharacters.charCodeAt(i); - } - const byteArray = new Uint8Array(byteNumbers); - const blob = new Blob([byteArray]); - formData.append("file", blob, options.filename); - } else { + // Handle file content + if (typeof options.file === "string") { + // Accept plain base64 or data URLs + const base64 = options.file.includes(";base64,") + ? options.file.split(";base64,").pop()! + : options.file; + const bytes = Uint8Array.from(atob(base64), (c) => c.charCodeAt(0)); + const blob = new Blob([bytes], { + type: options.filetype || "application/octet-stream", + }); + formData.append("file", blob, options.filename); + } else { formData.append("file", options.file, options.filename); }
310-324: Optional precondition: ensure payload has text or blocks.Avoid Slack API errors on empty messages.
async postMessage( channelId: string, text: string, opts: { thread_ts?: string; blocks?: unknown[] } = {}, ): Promise<{ @@ }> { + if (!text && !(opts.blocks && opts.blocks.length)) { + return { + ok: false, + error: "Either text or blocks must be provided", + channel: channelId, + ts: "", + message: {} as any, + }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
slack/actions/dms/send.ts(2 hunks)slack/actions/files/upload.ts(1 hunks)slack/client.ts(10 hunks)slack/loaders/conversations/open.ts(1 hunks)slack/loaders/dms/history.ts(1 hunks)slack/loaders/dms/list.ts(1 hunks)slack/loaders/files/list.ts(1 hunks)slack/manifest.gen.ts(1 hunks)slack/utils/client.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
slack/loaders/files/list.ts (1)
slack/client.ts (2)
SlackFile(130-185)listUserFiles(618-647)
slack/actions/files/upload.ts (2)
deno-deploy/client.ts (1)
File(81-85)slack/client.ts (1)
uploadFile(653-709)
slack/loaders/conversations/open.ts (3)
slack/loaders/dms/history.ts (1)
Props(4-15)slack/loaders/dms/list.ts (1)
Props(4-15)slack/loaders/files/list.ts (1)
Props(4-27)
slack/utils/client.ts (1)
slack/client.ts (4)
SlackResponse(7-14)SlackChannel(19-47)SlackMessage(52-107)SlackFile(130-185)
slack/client.ts (1)
deno-deploy/client.ts (1)
File(81-85)
🔇 Additional comments (5)
slack/loaders/dms/history.ts (1)
26-41: Verified: client response shapes consistent with usageopenDmChannel returns { ok: boolean; channel?: { id: string }; error?: string } (slack/client.ts:556–559). getChannelHistory returns SlackResponse<{ messages: SlackMessage[] }> (slack/client.ts:398–401; slack/loaders/channels/history.ts:21–27). The checks for .ok, .error and channel?.id are correct — no change required.
slack/loaders/files/list.ts (1)
66-71: LGTM overallGood defaults and error shaping on success path.
slack/actions/dms/send.ts (1)
46-49: Confirmed — chat.postMessage(userId) opens/resumes a 1:1 DM; ensure chat:writePassing a user ID ('U...') in the channel field will open/resume a 1:1 DM and post the message. Use a bot token with chat:write; if you explicitly call conversations.open, include im:write/mpim:write or the appropriate conversations.* write scopes as well.
slack/loaders/conversations/open.ts (1)
35-43: LGTM: response mapping mirrors Slack API (ok/no_op/already_open/channel/warning/response_metadata).slack/client.ts (1)
21-47: Type expansions align with Slack payloads.Optional fields and new entities (SlackBotProfile, extended SlackFile/SlackMessage) look good.
Also applies to: 54-107, 109-125, 140-141, 152-152, 162-185
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
slack/loaders/dms/history.ts (1)
33-38: Resolved: channelId lookup now covers both shapesGood fix using
channelResponse.data?.channel?.id ?? channelResponse.channel?.id; addresses the earlier bug where histories could be missed.
🧹 Nitpick comments (4)
slack/loaders/dms/history.ts (4)
35-38: Improve diagnostics when channel ID is missingAdd structured context (without dumping full payload/PII) to ease debugging across the new response shapes.
- if (!channelId) { - console.error("No channel ID returned for user", props.userId); - return []; - } + if (!channelId) { + console.error("No channel ID returned", { + userId: props.userId, + ok: channelResponse.ok, + warning: channelResponse.data?.warning ?? channelResponse.warning, + no_op: channelResponse.data?.no_op ?? channelResponse.no_op, + already_open: channelResponse.data?.already_open ?? channelResponse.already_open, + response_metadata: + channelResponse.data?.response_metadata ?? channelResponse.response_metadata, + }); + return []; + }
48-48: Harden access to messages to avoid rare NPEsGuard
dataas well, in case an upstream change returnsokwith nodata.- return historyResponse.data.messages || []; + return historyResponse.data?.messages ?? [];
48-48: Optional: expose pagination flags to align with client updatesIf consumers need paging parity with
getChannelHistory(has_more/next_cursor), consider returning them here.Example:
type DmHistoryResult = { messages: SlackMessage[]; has_more: boolean; next_cursor?: string; }; return { messages: historyResponse.data?.messages ?? [], has_more: historyResponse.data?.has_more ?? false, next_cursor: historyResponse.data?.response_metadata?.next_cursor, };(If you keep the current return type, ignore this.)
33-38: Cache IM channel IDs per user to reduce latency and rate-limit riskOpening every time adds an extra API call; cache
{userId -> channelId}(memory or persistent) and fall back toconversations.openon miss.Example:
// pseudo const cachedId = await ctx.cache.get<string>(`slack:im:${props.userId}`); const channelId = cachedId ?? (channelResponse.data?.channel?.id ?? channelResponse.channel?.id); if (!cachedId && channelId) await ctx.cache.set(`slack:im:${props.userId}`, channelId, { ttl: 86400 });
…sponse structures and pagination support
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
slack/utils/client.ts (3)
31-39: Remove response_metadata from inner data; keep it only at top-levelYou’re duplicating response_metadata under data. Keep it exclusively on SlackResponse.response_metadata.
- response: SlackResponse<{ + response: SlackResponse<{ channel: string; ts: string; message: SlackMessage; warning?: string; - response_metadata?: { - warnings?: string[]; - }; }>;If you need warnings, widen SlackResponse.response_metadata as suggested in slack/client.ts.
89-97: Drop inner response_metadata on conversations.openSame duplication issue; rely on the top-level envelope.
- response: SlackResponse<{ + response: SlackResponse<{ channel: { id: string }; no_op?: boolean; already_open?: boolean; warning?: string; - response_metadata?: { - warnings?: string[]; - }; }>;
116-125: files.upload: remove inner response_metadataMirror the approach above; keep warnings at top-level response_metadata.
- response: SlackResponse<{ + response: SlackResponse<{ file: SlackFile; - warning?: string; - response_metadata?: { - warnings?: string[]; - }; + warning?: string; }>;Also search for uses of
.data.response_metadatato ensure nothing breaks:#!/bin/bash rg -nP '\.data\.response_metadata' -g '!**/dist/**' -g '!**/build/**'
🧹 Nitpick comments (4)
slack/client.ts (4)
619-634: Tighten files.list filters typingConsider a union for types instead of free-form string to catch typos at compile time (Slack accepts comma-separated categories).
- async listUserFiles( + type SlackFileCategory = + | "all" | "spaces" | "snippets" | "images" | "gdocs" | "zips" | "pdfs"; + async listUserFiles( userId: string, count: number = 20, page: number = 1, - types: string = 'all' + types: SlackFileCategory | `${SlackFileCategory},${string}` = "all" )
654-669: Accept string[] for channelsSlack accepts comma-separated channel IDs; supporting string[] improves DX.
- async uploadFile(options: { - channels: string; + async uploadFile(options: { + channels: string | string[]; @@ - formData.append("channels", options.channels); + const channels = Array.isArray(options.channels) + ? options.channels.join(",") + : options.channels; + formData.append("channels", channels);
701-709: Add timeouts and basic 429 handling for uploadsNetwork calls lack timeouts and rate-limit backoff. Minimal guard:
- const response = await fetch("https://slack.com/api/files.upload", { + const ac = new AbortController(); + const t = setTimeout(() => ac.abort(), 20_000); + const response = await fetch("https://slack.com/api/files.upload", { method: "POST", headers: { Authorization: this.botHeaders.Authorization, }, - body: formData, + body: formData, + signal: ac.signal, }); + clearTimeout(t); + if (response.status === 429) { + // Slack sends Retry-After; surface a clear error to caller to retry. + const retryAfter = response.headers.get("retry-after") ?? undefined; + return { ok: false, error: `rate_limited${retryAfter ? `:${retryAfter}s` : ""}` } as any; + }
686-699: Handle data URLs and fail fast on empty/invalid base64 (slack/client.ts:686-699)Accept data: URIs, validate the base64 payload, and set a safe Blob type.
- if (typeof options.file === "string") { - // Assume base64 string, convert to Blob - const byteCharacters = atob(options.file); + if (typeof options.file === "string") { + // Accept raw base64 or data:...;base64,... + const b64 = options.file.includes("base64,") + ? options.file.split("base64,").pop()! + : options.file.trim(); + if (!b64) { + throw new Error("uploadFile: empty base64 payload"); + } + const byteCharacters = atob(b64); const byteNumbers = new Array(byteCharacters.length); for (let i = 0; i < byteCharacters.length; i++) { byteNumbers[i] = byteCharacters.charCodeAt(i); } const byteArray = new Uint8Array(byteNumbers); - const blob = new Blob([byteArray]); + const blob = new Blob([byteArray], { type: "application/octet-stream" }); formData.append("file", blob, options.filename);Runtime check: test with a small data:...;base64, URI and a plain base64 string to confirm both paths work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
slack/actions/files/upload.ts(1 hunks)slack/actions/messages/post.ts(1 hunks)slack/client.ts(8 hunks)slack/loaders/channels/history.ts(2 hunks)slack/loaders/dms/history.ts(2 hunks)slack/utils/client.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- slack/actions/files/upload.ts
- slack/loaders/dms/history.ts
🧰 Additional context used
🧬 Code graph analysis (4)
slack/actions/messages/post.ts (1)
slack/client.ts (1)
SlackMessage(52-107)
slack/loaders/channels/history.ts (1)
slack/client.ts (2)
SlackResponse(7-14)SlackMessage(52-107)
slack/utils/client.ts (1)
slack/client.ts (4)
SlackResponse(7-14)SlackChannel(19-47)SlackMessage(52-107)SlackFile(130-185)
slack/client.ts (1)
deno-deploy/client.ts (1)
File(81-85)
🔇 Additional comments (10)
slack/client.ts (3)
425-439: Good wrapper alignment with SlackResponseYou normalize Slack’s history payload into SlackResponse with data/messages and propagate response_metadata correctly. This matches your public contract.
558-573: DM open payload looks rightUsing { users } for conversations.open is correct for 1:1 DMs. Return shape includes no_op/already_open/warning which Slack may emit.
601-609: Consistent pagination shape for DMsWrapping channels under data and surfacing response_metadata at top-level is consistent with the history loader. LGTM.
slack/actions/messages/post.ts (1)
24-34: Return type widened — manual verification requiredMatches SlackClient.postMessage. Automated search found no callers accessing .data.channel or .data.ts; confirm loaders/other consumers don't expect the SlackResponse (.data.*) wrapper.
slack/loaders/channels/history.ts (3)
14-17: Cursor support added—goodPropagating cursor aligns with Slack pagination for conversations.history.
29-36: Richer payload passthroughReturning has_more/pin_count/channel_actions_* under data matches client changes.
37-39: Param threading is correctYou forward limit and cursor straight to the client; defaulting limit in the client keeps this clean.
slack/utils/client.ts (3)
20-22: LGTM: no duplicate response_metadataThis now returns SlackResponse<{ channels }>, leaving response_metadata at the top level. Consistent.
55-62: History shape looks rightNo inner response_metadata—good. Cursor added to searchParams—good.
100-115: files.list typing is cleanPaging under data, no duplicated response_metadata. LGTM.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
slack/loaders/dms/history.ts (1)
40-44: Use both shapes for channel lookup to avoid null ID when client wraps.Other loaders wrap payloads in
data. Check both places.- const channelId = channelResponse.channel?.id; + const channelId = + // If client returns SlackResponse + (channelResponse as any)?.data?.channel?.id ?? + // If client returns raw Slack shape + (channelResponse as any)?.channel?.id; if (!channelId) { - console.error("No channel ID returned for user", props.userId); + console.error("No channel ID returned for user", props.userId, channelResponse); return []; }
🧹 Nitpick comments (8)
slack/loaders/deco-chat/channels/list.ts (1)
23-25: Avoid empty labels; fall back to channel id (or other human-friendly fallback).An empty string can render as a blank option. Prefer a non-empty fallback to ensure usable UI.
Apply:
- label: ch.name ?? "", + label: ch.name ?? ch.id,Optional: if available in your typings, consider
ch.name ?? ch.name_normalized ?? ch.id.slack/client.ts (7)
10-12: Widen SlackResponse.response_metadata to carry warnings.Multiple endpoints (e.g., chat.postMessage, files.upload) return response_metadata.warnings. Typing it avoids ad-hoc fields elsewhere.
response_metadata?: { next_cursor?: string; + warnings?: string[]; };
70-107: Blocks typing is too narrow; consider relaxing to Slack Block Kit union or unknown[].The current nested shape won’t cover many valid blocks/elements and will cause type friction. Prefer the official Block types or simply
unknown[]to avoid false negatives until full types are introduced.- blocks?: Array<{ - type: string; - block_id: string; - elements: Array<{ - type: string; - elements?: Array<{ - type: string; - text: string; - }>; - }>; - }>; + blocks?: unknown[];
171-185: Make optional: channels/groups/ims/comments_count can be absent on several file shapes.Slack omits these on some responses; strict required typing will mislead consumers.
- channels: string[]; - groups: string[]; - ims: string[]; - comments_count: number; + channels?: string[]; + groups?: string[]; + ims?: string[]; + comments_count?: number;
567-585: Consider returning SlackResponse to match other helpers.openDmChannel now returns a raw Slack shape, while history/list methods wrap in SlackResponse. Unifying reduces consumer branching.
- async openDmChannel(userId: string): Promise<{ - ok: boolean; - channel?: { id: string }; - error?: string; - no_op?: boolean; - already_open?: boolean; - warning?: string; - response_metadata?: { - warnings?: string[]; - }; - }> { + async openDmChannel(userId: string): Promise< + SlackResponse<{ channel?: { id: string }; no_op?: boolean; already_open?: boolean; warning?: string }> + > { const response = await fetch("https://slack.com/api/conversations.open", { method: "POST", headers: this.botHeaders, body: JSON.stringify({ users: userId }), }); - - return response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { + channel: result.channel, + no_op: result.no_op, + already_open: result.already_open, + warning: result.warning, + }, + }; }
628-657: OK for now, but note shape divergence from other methods.listUserFiles returns a raw Slack shape (ok/files/paging) while most client methods use SlackResponse. If you later standardize, this is a candidate.
695-707: Handle data URLs and avoid O(n) manual loop in base64 decode.Accept both raw base64 and data URLs; use Uint8Array.from(atob(...)) for speed; also preserve MIME type if present.
- if (typeof options.file === "string") { - // Assume base64 string, convert to Blob - const byteCharacters = atob(options.file); - const byteNumbers = new Array(byteCharacters.length); - for (let i = 0; i < byteCharacters.length; i++) { - byteNumbers[i] = byteCharacters.charCodeAt(i); - } - const byteArray = new Uint8Array(byteNumbers); - const blob = new Blob([byteArray]); - formData.append("file", blob, options.filename); - } else { + if (typeof options.file === "string") { + // Accept raw base64 or data URL: data:<mime>;base64,<data> + let input = options.file; + let mime: string | undefined; + const m = /^data:([^;]+);base64,/.exec(input); + if (m) { + mime = m[1]; + input = input.slice(m[0].length); + } + const bytes = Uint8Array.from(atob(input), (c) => c.charCodeAt(0)); + const blob = new Blob([bytes], mime ? { type: mime } : undefined); + formData.append("file", blob, options.filename); + } else { formData.append("file", options.file, options.filename); }
660-678: Optional: propagate warnings/metadata in a normalized envelope.files.upload returns warning and response_metadata.warnings. Consider SlackResponse for parity (postMessage/history). If keeping raw shape, current typing is fine.
Also applies to: 710-719
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
slack/actions/deco-chat/channels/invoke.ts(1 hunks)slack/actions/deco-chat/channels/join.ts(1 hunks)slack/actions/deco-chat/channels/leave.ts(1 hunks)slack/actions/dms/send.ts(1 hunks)slack/actions/files/upload.ts(1 hunks)slack/client.ts(8 hunks)slack/loaders/conversations/open.ts(1 hunks)slack/loaders/deco-chat/channels/list.ts(2 hunks)slack/loaders/dms/history.ts(2 hunks)slack/loaders/dms/list.ts(1 hunks)slack/loaders/files/list.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- slack/actions/deco-chat/channels/invoke.ts
- slack/loaders/dms/list.ts
- slack/actions/deco-chat/channels/leave.ts
- slack/actions/deco-chat/channels/join.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- slack/loaders/conversations/open.ts
- slack/actions/files/upload.ts
- slack/loaders/files/list.ts
- slack/actions/dms/send.ts
🧰 Additional context used
🧬 Code graph analysis (1)
slack/client.ts (3)
slack/loaders/channels/history.ts (1)
getChannelHistory(25-39)slack/loaders/files/list.ts (1)
listUserFiles(46-82)slack/actions/files/upload.ts (1)
uploadFile(52-87)
🪛 Biome (2.1.2)
slack/client.ts
[error] 408-408: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 409-409: expected , but instead found :
Remove :
(parse)
[error] 410-410: expected , but instead found limit
Remove limit
(parse)
[error] 410-410: expected , but instead found :
Remove :
(parse)
[error] 411-411: expected , but instead found cursor
Remove cursor
(parse)
[error] 411-411: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 412-412: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 413-413: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 414-414: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 415-415: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 416-416: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 417-417: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 418-418: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 419-419: Expected a statement but instead found '>>'.
Expected a statement here.
(parse)
[error] 435-447: Illegal return statement outside of a function
(parse)
[error] 567-567: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 567-567: expected , but instead found :
Remove :
(parse)
[error] 567-567: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 569-569: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 570-570: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 571-571: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 572-572: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 573-573: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 574-574: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 575-575: expected , but instead found ?
Remove ?
(parse)
[error] 575-575: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 575-575: expected , but instead found ;
Remove ;
(parse)
[error] 577-577: Expected a statement but instead found '}>'.
Expected a statement here.
(parse)
[error] 584-584: Illegal return statement outside of a function
(parse)
[error] 611-618: Illegal return statement outside of a function
(parse)
[error] 628-628: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 629-629: expected , but instead found :
Remove :
(parse)
[error] 630-630: expected , but instead found count
Remove count
(parse)
[error] 630-630: expected , but instead found :
Remove :
(parse)
[error] 631-631: expected , but instead found page
Remove page
(parse)
[error] 631-631: expected , but instead found :
Remove :
(parse)
[error] 632-632: expected , but instead found types
Remove types
(parse)
[error] 632-632: expected , but instead found :
Remove :
(parse)
[error] 633-633: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 635-635: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 642-642: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 643-643: Expected a statement but instead found '>'.
Expected a statement here.
(parse)
[error] 656-656: Illegal return statement outside of a function
(parse)
[error] 663-663: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 663-663: expected , but instead found :
Remove :
(parse)
[error] 664-664: expected , but instead found channels
Remove channels
(parse)
[error] 664-664: expected , but instead found :
Remove :
(parse)
[error] 664-664: expected , but instead found ;
Remove ;
(parse)
[error] 667-667: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 668-668: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 669-669: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 670-670: Expected a statement but instead found '}): Promise<'.
Expected a statement here.
(parse)
[error] 672-672: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 673-673: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 674-674: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 675-675: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 676-676: expected , but instead found ?
Remove ?
(parse)
[error] 676-676: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 676-676: expected , but instead found ;
Remove ;
(parse)
[error] 678-678: Expected a statement but instead found '}>'.
Expected a statement here.
(parse)
[error] 718-718: Illegal return statement outside of a function
(parse)
🪛 GitHub Actions: ci
slack/client.ts
[error] 334-334: Error formatting: Unexpected token payload. Expected * for generator, private key, identifier or async.
🔇 Additional comments (6)
slack/loaders/deco-chat/channels/list.ts (1)
10-10: Doc metadata addition looks good.Adding @title improves discoverability in tooling; no runtime impact.
slack/client.ts (2)
406-419: LGTM: history cursor support and unified SlackResponse wrapper.Nice normalization of the shape and pass-through of has_more and warning. No issues.
Also applies to: 425-427, 434-447
610-618: LGTM: listDmChannels normalized to SlackResponse with response_metadata.Consistent with getChannelHistory; good.
slack/loaders/dms/history.ts (3)
16-20: LGTM: added cursor support to Props.Pagination for DM history aligns with client.getChannelHistory.
47-47: LGTM: pass cursor through to getChannelHistory.Matches the client signature.
54-54: LGTM: unwrap messages from SlackResponse.data.Correct extraction; aligns with client history response.
…tructures and optional fields
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
slack/client.ts (3)
260-285: Unify return shape to SlackResponse for consistencyThis method returns raw Slack JSON, unlike others that wrap with SlackResponse. Standardize to avoid caller inconsistencies.
- ): Promise< - { channels: SlackChannel[]; response_metadata?: { next_cursor?: string } } - > { + ): Promise<SlackResponse<{ channels: SlackChannel[] }>> { @@ - return await response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { + channels: result.channels || [], + }, + };
480-503: Expose pagination on conversations.repliesAdd cursor (and optional limit) to avoid truncation for long threads.
- async getThreadReplies( - channelId: string, - threadTs: string, - ): Promise<SlackResponse<{ messages: SlackMessage[] }>> { + async getThreadReplies( + channelId: string, + threadTs: string, + cursor?: string, + limit: number = 100, + ): Promise<SlackResponse<{ messages: SlackMessage[]; has_more?: boolean }>> { @@ - const params = new URLSearchParams({ + const params = new URLSearchParams({ channel: channelId, ts: threadTs, + limit: Math.min(limit, 200).toString(), }); + if (cursor) params.append("cursor", cursor); @@ - return { + return { ok: result.ok, error: result.error, response_metadata: result.response_metadata, data: { - messages: result.messages || [], + messages: result.messages || [], + has_more: result.has_more, }, };
239-252: Add a tiny fetch helper with timeouts and 429 retryReduce duplication and improve resiliency (timeouts, basic 429 backoff, consistent JSON parse).
// helper (place as private method in SlackClient) private async request(path: string, init: RequestInit & { timeoutMs?: number } = {}) { const { timeoutMs = 15000, ...rest } = init; const ctrl = new AbortController(); const t = setTimeout(() => ctrl.abort(), timeoutMs); let res: Response; try { res = await fetch(path, { signal: ctrl.signal, ...rest }); } finally { clearTimeout(t); } // Basic 429 handling if (res.status === 429) { const retryAfter = Number(res.headers.get("retry-after") ?? 1); await new Promise(r => setTimeout(r, retryAfter * 1000)); return this.request(path, init); } return res.json(); }Then replace repeated fetch/json blocks with
const result = await this.request(url, init);.Also applies to: 338-345, 371-379, 405-414, 454-461, 489-497, 525-533, 552-560, 572-580, 612-619, 641-648, 679-686, 723-731, 793-801
🧹 Nitpick comments (6)
slack/client.ts (6)
255-259: Docstring mismatch with default typesDocs say “public channels” but default types include private_channel as well. Update wording or default types for accuracy.
- * @description Lists all public channels in the workspace + * @description Lists conversations (public/private) in the workspace
323-356: Add precondition: require text or blocksFail fast client-side to surface clearer errors before hitting Slack validation.
const payload: Record<string, unknown> = { channel: channelId, text: text, ...opts, }; + if (!text && (!opts.blocks || opts.blocks.length === 0)) { + throw new Error("chat.postMessage requires either `text` or non-empty `blocks`."); + } // Remove text if blocks are provided and text is empty (Slack requires at least one of them) if (opts.blocks && opts.blocks.length > 0 && !text) { delete payload.text; }
371-391: Minor duplication: consider reusing postMessage for repliespostReply can delegate to postMessage with { thread_ts } to reduce surface area and keep behavior aligned.
597-629: Mirror postMessage precondition in updateMessageEnforce text or blocks to avoid Slack-side errors.
if (opts.blocks && opts.blocks.length > 0 && !text) { delete payload.text; } + if (!payload["text"] && (!opts.blocks || opts.blocks.length === 0)) { + throw new Error("chat.update requires either `text` or non-empty `blocks`."); + }
635-659: Support multi-user DMs in openDmChannelAllow string | string[] and join when array is provided.
-async openDmChannel(userId: string): Promise<SlackResponse<{ +async openDmChannel(userId: string | string[]): Promise<SlackResponse<{ @@ - body: JSON.stringify({ users: userId }), + body: JSON.stringify({ users: Array.isArray(userId) ? userId.join(",") : userId }),
702-743: Confirm: Slack files.list uses page/count (traditional pagination), not cursor-based.
Keep the current page/count implementation — response includes paging.{count,total,page,pages}. If you need cursor-based pagination for consistency, use files.remote.list (supports cursor+limit and response_metadata.next_cursor).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
slack/client.ts(18 hunks)
🔇 Additional comments (11)
slack/client.ts (11)
12-13: LGTM: propagate Slack warnings via response_metadataGood addition; aligns with Slack’s payloads and avoids duplicating warnings in data.
22-48: LGTM: DM-specific channel fields addedOptional DM fields on SlackChannel look correct and non-breaking.
110-126: LGTM: SlackBotProfile typeMatches Slack schema expectations and is properly referenced from SlackMessage.
153-186: LGTM: Expanded SlackFile thumbnails and metadataBroader thumb sizes and optional flags are accurate and helpful for clients.
302-310: LGTM: conversations.join wrappedConsistent with new SlackResponse envelope.
415-424: LGTM: reactions.add with safe fallbacksGraceful defaults for channel/ts are practical.
431-472: LGTM: history pagination and richer fieldsCursor support and additional flags mapped correctly.
529-537: LGTM: users.list mappingLimit capping and envelope are correct.
557-565: LGTM: users.profile.get mappingIncludes labels and wraps in SlackResponse.
576-583: LGTM: auth.test passthroughReturning the whole payload under data is appropriate.
684-692: LGTM: IM listing envelopeConsistent with conversations.list for types=im.
| blocks?: Array<{ | ||
| type: string; | ||
| block_id: string; | ||
| elements: Array<{ | ||
| type: string; | ||
| elements?: Array<{ | ||
| type: string; | ||
| text: string; | ||
| }>; | ||
| }>; | ||
| }>; | ||
| subtype?: string; | ||
| edited?: { | ||
| user: string; | ||
| ts: string; | ||
| }; | ||
| team?: string; | ||
| bot_id?: string; | ||
| app_id?: string; | ||
| bot_profile?: SlackBotProfile; | ||
| assistant_app_thread?: { | ||
| title: string; | ||
| title_blocks: Array<{ | ||
| type: string; | ||
| block_id: string; | ||
| elements: Array<{ | ||
| type: string; | ||
| elements: Array<{ | ||
| type: string; | ||
| text: string; | ||
| }>; | ||
| }>; | ||
| }>; | ||
| artifacts: unknown[]; | ||
| context: Record<string, unknown>; | ||
| }; | ||
| [key: string]: unknown; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer official Block Kit types over ad-hoc shapes
Replace custom block element shapes with Slack’s KnownBlock/Block/BlockElement types (from @slack/types) to prevent drift and improve tooling.
Example:
- blocks?: Array<{
- type: string;
- block_id: string;
- elements: Array<{
- type: string;
- elements?: Array<{
- type: string;
- text: string;
- }>;
- }>;
- }>;
+ blocks?: import("@slack/types").KnownBlock[];📝 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.
| blocks?: Array<{ | |
| type: string; | |
| block_id: string; | |
| elements: Array<{ | |
| type: string; | |
| elements?: Array<{ | |
| type: string; | |
| text: string; | |
| }>; | |
| }>; | |
| }>; | |
| subtype?: string; | |
| edited?: { | |
| user: string; | |
| ts: string; | |
| }; | |
| team?: string; | |
| bot_id?: string; | |
| app_id?: string; | |
| bot_profile?: SlackBotProfile; | |
| assistant_app_thread?: { | |
| title: string; | |
| title_blocks: Array<{ | |
| type: string; | |
| block_id: string; | |
| elements: Array<{ | |
| type: string; | |
| elements: Array<{ | |
| type: string; | |
| text: string; | |
| }>; | |
| }>; | |
| }>; | |
| artifacts: unknown[]; | |
| context: Record<string, unknown>; | |
| }; | |
| [key: string]: unknown; | |
| } | |
| blocks?: import("@slack/types").KnownBlock[]; | |
| subtype?: string; | |
| edited?: { | |
| user: string; | |
| ts: string; | |
| }; | |
| team?: string; | |
| bot_id?: string; | |
| app_id?: string; | |
| bot_profile?: SlackBotProfile; | |
| assistant_app_thread?: { | |
| title: string; | |
| title_blocks: Array<{ | |
| type: string; | |
| block_id: string; | |
| elements: Array<{ | |
| type: string; | |
| elements: Array<{ | |
| type: string; | |
| text: string; | |
| }>; | |
| }>; | |
| }>; | |
| artifacts: unknown[]; | |
| context: Record<string, unknown>; | |
| }; | |
| [key: string]: unknown; | |
| } |
🤖 Prompt for AI Agents
In slack/client.ts around lines 71 to 108 the file defines ad-hoc shapes for
Block Kit payloads; replace those with Slack’s official types by importing
KnownBlock, Block, and BlockElement (or the appropriate types) from @slack/types
and use them for the blocks, title_blocks and nested element fields; update
assistant_app_thread to reference arrays of KnownBlock/Block/BlockElement where
applicable, keep other custom fields (artifacts, context) and the index
signature ([key: string]: unknown) intact, and run TypeScript to fix any
narrowed unions or nullable cases (e.g., make fields optional or union with
unknown) so callers compile without changing runtime behavior.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
slack/client.ts (3)
265-285: Unify return envelope: wrap getChannels in SlackResponseOther methods return SlackResponse; this one returns raw Slack JSON. Suggest wrapping for consistency.
async getChannels( teamId: string, limit: number = 1000, cursor?: string, types: ChannelType[] = ["public_channel", "private_channel"], - ): Promise< - { channels: SlackChannel[]; response_metadata?: { next_cursor?: string } } - > { + ): Promise<SlackResponse<{ channels: SlackChannel[] }>> { @@ - return await response.json(); + const result = await response.json(); + return { + ok: result.ok, + error: result.error, + response_metadata: result.response_metadata, + data: { + channels: result.channels || [], + }, + };
322-336: Narrow blocks type to KnownBlock[]Improves type safety for callers and payload construction.
- opts: { thread_ts?: string; blocks?: unknown[] } = {}, + opts: { thread_ts?: string; blocks?: import("@slack/types").KnownBlock[] } = {},
480-487: Add pagination to getThreadRepliesconversations.replies is cursor‑paginated; exposing limit/cursor avoids large threads truncation.
async getThreadReplies( channelId: string, threadTs: string, - ): Promise<SlackResponse<{ messages: SlackMessage[] }>> { + limit: number = 100, + cursor?: string, + ): Promise<SlackResponse<{ messages: SlackMessage[] }>> { - const params = new URLSearchParams({ - channel: channelId, - ts: threadTs, - }); + const params = new URLSearchParams({ + channel: channelId, + ts: threadTs, + limit: Math.min(limit, 1000).toString(), + }); + if (cursor) params.append("cursor", cursor);Also applies to: 494-502
♻️ Duplicate comments (3)
slack/client.ts (3)
71-81: Use official Block Kit types instead of ad‑hoc shapesReplace the custom blocks shape with @slack/types KnownBlock[]. This prevents drift and improves editor tooling.
- blocks?: Array<{ - type: string; - block_id: string; - elements: Array<{ - type: string; - elements?: Array<{ - type: string; - text: string; - }>; - }>; - }>; + blocks?: import("@slack/types").KnownBlock[];
92-106: Also type assistant_app_thread.title_blocks as KnownBlock[]Keeps Block Kit typing consistent across the file.
- title_blocks: Array<{ - type: string; - block_id: string; - elements: Array<{ - type: string; - elements: Array<{ - type: string; - text: string; - }>; - }>; - }>; + title_blocks: import("@slack/types").KnownBlock[];
596-601: Narrow updateMessage blocks type to KnownBlock[]Same rationale as postMessage.
- opts: { thread_ts?: string; blocks?: unknown[] } = {}, + opts: { thread_ts?: string; blocks?: import("@slack/types").KnownBlock[] } = {},
🧹 Nitpick comments (2)
slack/client.ts (2)
22-48: Be deliberate about making SlackChannel fields optionalLoosening many core fields (name, is_channel, creator, members, etc.) to optional can mask breaking API/consumer changes at compile time. Consider a discriminated union for IM vs non‑IM (e.g., SlackImChannel | SlackStandardChannel) so required fields remain required per kind.
702-743: listUserFiles — LGTM; consider optional time filtersfiles.list supports ts_from/ts_to and types filters; exposing the time window can reduce pages for large accounts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
slack/client.ts(18 hunks)
🔇 Additional comments (13)
slack/client.ts (13)
10-13: Include warnings in response_metadata — LGTMPropagating response_metadata.warnings aligns with Slack responses and helps surface soft failures.
110-126: SlackBotProfile shape — LGTM
141-186: SlackFile expansion — LGTMFields match Slack payloads and keep thumbnails optional.
344-355: postMessage: wrapped response — LGTMEnvelope and field mapping look consistent with other helpers.
381-391: postReply wrapper — LGTM
415-424: addReaction payload/response — LGTMJSON body with fallback channel/ts is fine; Slack often returns only ok.
431-473: getChannelHistory with cursor — LGTMParameters and mapped fields align with conversations.history responses.
529-537: getUsers wrapper — LGTM
557-565: getUserProfile wrapper — LGTM
576-583: testAuth wrapper — LGTM
618-628: updateMessage return envelope — LGTM
635-659: openDmChannel response — LGTMusers payload and flags (no_op, already_open) match API behavior.
684-692: listDmChannels wrapper — LGTM
…ncx/apps-mcp into feat/slack-improvements
…n DM and file listing actions
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
slack/actions/dms/send.ts (1)
15-18: Typeblockswith official Block Kit typesUse Slack’s KnownBlock to avoid drift.
- blocks?: unknown[]; + blocks?: import("@slack/types").KnownBlock[];
♻️ Duplicate comments (4)
slack/client.ts (4)
71-108: Prefer official Block Kit types (repeat)Replace ad‑hoc
blocksand nested shapes withKnownBlock[]to prevent type drift. Also updateassistant_app_thread.title_blocks.- blocks?: Array<{ - type: string; - block_id: string; - elements: Array<{ - type: string; - elements?: Array<{ - type: string; - text: string; - }>; - }>; - }>; + blocks?: import("@slack/types").KnownBlock[]; @@ - assistant_app_thread?: { + assistant_app_thread?: { title: string; - title_blocks: Array<{ - type: string; - block_id: string; - elements: Array<{ - type: string; - elements: Array<{ - type: string; - text: string; - }>; - }>; - }>; + title_blocks: import("@slack/types").KnownBlock[]; artifacts: unknown[]; context: Record<string, unknown>; };
961-963: Same base64 decode issue in legacy uploadApply the same fallback here for parity.
- const bytes = Uint8Array.from(atob(input), (c) => c.charCodeAt(0)); + const bin = (typeof atob === "function") + ? atob(input) + // @ts-ignore Node fallback + : Buffer.from(input, "base64").toString("binary"); + const bytes = Uint8Array.from(bin, (c) => c.charCodeAt(0));
358-367: AlignpostMessageopts with Block Kit typesUpdate
blocksparam type toKnownBlock[].- opts: { thread_ts?: string; blocks?: unknown[] } = {}, + opts: { thread_ts?: string; blocks?: import("@slack/types").KnownBlock[] } = {},
818-821: Make base64 decode Node/Deno‑safe (V2 upload)
atobisn’t defined in Node; add a safe fallback.- const bytes = Uint8Array.from(atob(input), (c) => c.charCodeAt(0)); + const bin = (typeof atob === "function") + ? atob(input) + // @ts-ignore Node fallback + : Buffer.from(input, "base64").toString("binary"); + const bytes = Uint8Array.from(bin, (c) => c.charCodeAt(0));
🧹 Nitpick comments (8)
slack/actions/dms/send.ts (2)
43-51: Guard: require text or blocksFail fast when both are missing; avoid Slack errors.
export default async function sendDm( props: SendDmProps, _req: Request, ctx: AppContext, ): Promise<SendDmResponse> { + if (!props.text && (!props.blocks || props.blocks.length === 0)) { + return { success: false, message: "Provide text or blocks." }; + } try {
9-18: Optional thread supportDMs can be threaded; make
thread_tsopt-in and forward it.text: string; /** * @description Optional blocks for Block Kit formatting */ - blocks?: unknown[]; + blocks?: import("@slack/types").KnownBlock[]; + /** + * @description Optional thread timestamp + */ + thread_ts?: string; } @@ - const messageResponse = await ctx.slack.postMessage(props.userId, props.text, { - blocks: props.blocks, - }); + const messageResponse = await ctx.slack.postMessage( + props.userId, + props.text, + { blocks: props.blocks, thread_ts: props.thread_ts }, + );Also applies to: 49-51
slack/loaders/channels.ts (1)
61-66: Optionally return aggregated warningsYou could bubble up any
response_metadata.warningsencountered to aid ops visibility.slack/actions/files/uploadV2.ts (1)
24-33: Enforce: thread_ts requires channelsAvoid a Slack error by validating inputs.
export interface UploadFileV2Props { @@ initial_comment?: string; } @@ - if (!response.ok) { + if (props.thread_ts && !props.channels) { + return { ok: false, files: [], error: "thread_ts requires channels" }; + } + if (!response.ok) { return { ok: false, files: [], error: response.error || "Failed to upload file", }; }Also applies to: 73-79
slack/actions/files/upload.ts (4)
2-2: Make SlackFile a type‑only import.Avoids unnecessary runtime import/bundle impact.
-import { SlackFile } from "../../client.ts"; +import type { SlackFile } from "../../client.ts";
115-120: Accept Blob for legacy by converting to File (better UX).You currently reject Blob; convert instead so callers don’t have to branch.
- if (typeof props.file !== "string" && !(props.file instanceof File)) { + if (typeof props.file !== "string" && !(props.file instanceof File)) { + if (props.file instanceof Blob) { + const asFile = new File([props.file], props.filename, { type: props.file.type || "application/octet-stream" }); + props = { ...props, file: asFile }; + } else { return { ok: false, error: "Legacy API only supports string (base64) or File objects. Use V2 API for other file types.", }; - } + } }
131-136: Normalize error shape across paths.Legacy error branch omits files; V2 returns files: []. Align for simpler callers.
if (!response.ok) { return { ok: false, - error: response.error || "Failed to upload file with legacy API", + files: [], + error: response.error || "Failed to upload file with legacy API", }; }
72-77: Consider returning SlackResponse for consistency with other actions.Most actions now return SlackResponse. Wrapping UploadFileResponse similarly would standardize error/metadata handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
slack/README.md(2 hunks)slack/actions/dms/send.ts(1 hunks)slack/actions/files/upload.ts(1 hunks)slack/actions/files/uploadV2.ts(1 hunks)slack/actions/messages/post.ts(3 hunks)slack/client.ts(20 hunks)slack/loaders/channels.ts(3 hunks)slack/loaders/files/list.ts(1 hunks)slack/manifest.gen.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- slack/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- slack/loaders/files/list.ts
🧰 Additional context used
🧬 Code graph analysis (5)
slack/actions/files/uploadV2.ts (1)
slack/client.ts (1)
uploadFileV2(787-907)
slack/actions/messages/post.ts (1)
slack/client.ts (2)
SlackResponse(7-15)SlackMessage(53-108)
slack/actions/dms/send.ts (2)
figma/mod.ts (1)
AppContext(17-17)vtex/mod.ts (1)
AppContext(25-25)
slack/loaders/channels.ts (1)
slack/client.ts (2)
SlackResponse(7-15)SlackChannel(20-48)
slack/actions/files/upload.ts (1)
slack/client.ts (2)
SlackFile(131-186)uploadFile(914-986)
🔇 Additional comments (8)
slack/actions/dms/send.ts (1)
48-52: Directly posting to a user ID is validPassing a user ID as
channelis supported; Slack will open the DM if needed. Expectchannel_not_foundif the app can’t DM that user. Good call. (api.slack.com)slack/client.ts (1)
910-913: Deprecation notice date is correctfiles.upload sunsets on November 12, 2025. Keeping both methods is prudent.
slack/loaders/channels.ts (1)
40-78: Pagination loop looks solidAccumulates pages via
next_cursor, exits cleanly, and propagates errors.slack/manifest.gen.ts (2)
9-11: Wiring for new loaders/actions looks correctNew uploads (V2 + legacy) and conversations/open are registered; removed modules aren’t referenced here.
Also applies to: 18-41, 47-49
1-61: Verify no stale references to removed Slack file actions — search inconclusiverg produced no output; this is inconclusive. Confirm there are no remaining imports/usages of: files/download.ts, files/info.ts, downloadFile, or getFileInfo (or run a repo-wide search from the project root and paste the results).
slack/actions/files/uploadV2.ts (1)
63-86: Happy path + error propagation LGTMDelegates to client, preserves warnings, and standardizes errors.
slack/actions/files/upload.ts (1)
80-88: No change required — uploadFileV2 useschannelsand maps tochannel_id.
uploadFileV2 acceptschannelsand setscompletePayload.channel_id = options.channelsin slack/client.ts (~lines 879–881), so callers passingchannelsare correct.slack/actions/messages/post.ts (1)
32-40: Confirmed — ctx.slack.postMessage signature and return type match usage.
slack/client.ts (≈lines 357–366) defines postMessage(channelId: string, text: string, opts: { thread_ts?: string; blocks?: unknown[] } = {}) => Promise<SlackResponse<{ channel: string; ts: string; message: SlackMessage; warning?: string }>>.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… SlackFile for compatibility
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
slack/actions/files/upload.ts (1)
2-2: Use type‑only import for SlackFile.Avoids unnecessary runtime import/side‑effects.
Apply this diff:
-import { SlackFile } from "../../client.ts"; +import type { SlackFile } from "../../client.ts";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
slack/actions/files/upload.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
slack/actions/files/upload.ts (1)
slack/client.ts (2)
SlackFile(131-186)uploadFile(914-986)
🔇 Additional comments (1)
slack/actions/files/upload.ts (1)
47-57: No action needed — uploadFileV2 files shape matches the expected subset.uploadFileV2 (slack/client.ts) and SlackCompleteUploadResponse return files with id, name, title, mimetype, filetype, permalink, url_private; slack/actions/files/upload.ts maps response.data.files (and builds the compatibility
filepick) accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
What is this Contribution About?
DM Management Enhancements
conversations/open.ts- Opens DM channels with users usingconversations.openAPIdms/list.tswith proper pagination handling viaresponse_metadata.next_cursordms/history.tswith direct message access from API response and added cursor-based pagination supportdms/send.tsto send messages directly to user IDs without pre-opening channelsFile Operations Overhaul
files/list.ts- Lists files by user with pagination support usingfiles.listAPIfiles/upload.ts- Uploads files with FormData support for base64 strings and File objectsfiles/download.tsandfiles/info.tsas they were not being used effectivelyChannel Operations Improvements
channels/history.tswith cursor-based pagination support and complete API response structuremessages/post.tsto return complete API response with warnings and metadata🔧 Technical Improvements
API-Aligned Typings
is_im,is_org_shared,context_team_id,user,priority)subtype,assistant_app_thread,blocks,edited,reply_count,bot_profile)thumb_720,thumb_800,thumb_960,thumb_1024),media_display_type,user_team,comments_countClient Method Updates
response_metadataat top level without duplicationno_op,already_open,warning,response_metadatafieldshas_more,pin_count,channel_actions_ts,warningfields and cursor pagination supportResponse Structure Improvements
response_metadataduplication insidedataobjects - now only at top level ofSlackResponseOAuth Scopes Enhancement
assistant:write,incoming-webhook,mpim:read) to align with actual bot permissions📋 API Evidence Integration
All changes are based on real Slack API responses:
conversations.list?types=imfor DM channel listingconversations.historyfor message history with cursor paginationconversations.openfor opening DM channelschat.postMessagefor sending messages with complete response handlingfiles.list?user=USER_IDfor file listing with count-based paginationfiles.uploadfor file uploads with FormData support🗑️ Cleanup & Consistency
getFileInfo,downloadFile)SlackFileinstead of duplicating in upload response)🎯 Benefits
📚 Usage Examples
Send DM directly to user
List user files with pagination
Upload file to DM
Get channel history with pagination
Open DM conversation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor