Add channel-specific bot routing for Slack app #1435
Conversation
Introduces channel-specific bot routing, allowing configuration of different bots per channel. Adds bot routing actions, loaders, types, and utilities. Updates Slack actions to use channel-resolved bot clients, and enhances documentation with usage, migration, and troubleshooting details.
Tagging OptionsShould a new tag be published when this PR is merged?
|
|
Warning Rate limit exceeded@kevinicolas22 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 10 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)
WalkthroughAdds channel-specific Slack bot routing: new types and BotRouter utility, app init updates for per-channel Slack clients, actions/loaders to set/remove/list/get/compare bot configs, message/file actions switched to channel-scoped clients, and extensive README and manifest updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller (action/loader)
participant App as Slack App Core
participant Router as BotRouter
participant Client as SlackClient
Caller->>App: invoke with channelId (or channelIds)
App->>Router: resolveForChannel(channelId)
Router-->>App: ResolvedBotConfig (config, isDefault)
App->>App: build channel-scoped props (token/clientId/secret/customBotName)
App->>Client: slackClientForChannel(channelId) → client.call (post/upload/react/reply)
Client-->>App: Slack API response
App-->>Caller: return result (redacted when exposing config)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
slack/mod.ts (2)
160-182: Token refresh may persist channel-scoped overrides into app config.onTokenRefresh calls ctx.configure({ ...props, tokens }), where props may be channelProps (from slackClientForChannel). This can unintentionally set app config (e.g., customBotName) to a channel-specific value.
Use base appProps instead:
- onTokenRefresh: async (newTokens: OAuthTokens) => { - if (ctx) { - await ctx.configure({ - ...props, - tokens: newTokens, - }); - } - }, + onTokenRefresh: async (newTokens: OAuthTokens) => { + if (ctx) { + await ctx.configure({ + ...appProps, + tokens: newTokens, + }); + } + },Also applies to: 189-205
206-224: Verify and update call sites to use channel-specific routing instead of global ctx.slack.Verification confirms remaining usages exist across 11 files:
- Actions (bypass routing risk): slack/actions/deco-chat/channels/join.ts (lines 25, 32), slack/actions/dms/send.ts (lines 41, 69)
- Loaders: slack/loaders/channels/history.ts:40, slack/loaders/channels.ts:46+80, slack/loaders/conversations/open.ts:29, slack/loaders/dms/history.ts:42+64, slack/loaders/dms/list.ts:29, slack/loaders/files/list.ts:50, slack/loaders/thread/replies.ts:26, slack/loaders/user/profile.ts:22, slack/loaders/users.ts:35
Update these to use
slackClientForChannelor equivalent per-channel routing mechanism to ensure actions respect channel boundaries.slack/actions/files/upload.ts (1)
60-70: Routing fix is correct but incomplete—uploadFileV2 call also needs updating.The web search confirms that files.uploadV2 uses a single channel_id rather than comma-separated channels. The proposed routing fix (line 61) correctly extracts the first channel, but line 64 passes
channels: props.channels(comma-separated) directly touploadFileV2, which violates the API contract.Both locations need fixing:
- const slackClient = ctx.slackClientForChannel(props.channels); + const targetChannel = props.channels.split(",")[0].trim(); + const slackClient = ctx.slackClientForChannel(targetChannel); return await slackClient.uploadFileV2({ - channels: props.channels, + channels: targetChannel,
🧹 Nitpick comments (14)
slack/types/bot-routing.ts (1)
86-94: Key vs. value duplication in channelBots.channelBots is keyed by channelId and ChannelBotConfig also carries channelId. Consider asserting consistency or deriving channelId from the key to avoid drift.
slack/actions/messages/react.ts (1)
32-35: Normalize reaction names to reduce Slack API errors.Users often pass “:thumbsup:”. Strip colons and trim before calling the API.
- const slackClient = ctx.slackClientForChannel(channelId); - - return await slackClient.addReaction(channelId, timestamp, reaction); + const slackClient = ctx.slackClientForChannel(channelId); + const name = reaction.replace(/:/g, "").trim(); + return await slackClient.addReaction(channelId, timestamp, name);slack/actions/messages/threads/reply.ts (1)
32-36: Trim reply text to avoid accidental empty/whitespace posts.- const slackClient = ctx.slackClientForChannel(channelId); - - return await slackClient.postReply(channelId, threadTs, text); + const slackClient = ctx.slackClientForChannel(channelId); + const body = text.trim(); + return await slackClient.postReply(channelId, threadTs, body);slack/actions/messages/post.ts (1)
41-45: Prefer strong typing for Block Kit.blocks?: unknown[] weakens safety. If feasible, use Slack’s Block types (e.g., KnownBlock | Block) to catch schema issues at compile time.
slack/mod.ts (2)
189-205: Consider memoizing per-channel clients.slackClientForChannel creates a new SlackClient on every call. If actions are hot, memoize by channelId (and invalidate on routing changes) to save allocations.
143-149: Default routing when teamId/customBotName are missing.createDefaultBotRouting uses empty teamId and falls back bot name. Confirm that downstream logic (IDs like
default-) won’t collide across tenants.slack/README.md (3)
412-420: Deduplicate repeated migration sentence.The line “If you're upgrading from a previous version that only supported a single bot configuration:” appears twice.
-If you're upgrading from a previous version that only supported a single bot configuration: - If you're upgrading from a previous version that only supported a single bot configuration:
536-548: Hyphenate “single-bot” for consistency.Use “single-bot” (hyphenated) alongside “multi-bot”.
-1. Test both single bot and multi-bot routing scenarios +1. Test both single-bot and multi-bot routing scenarios
196-236: Add an explicit security note: loader responses are redacted.Since channel configs may include secrets (botToken, clientSecret), document that list/get/compare loaders return redacted configs and never expose secrets.
Proposed snippet to insert after “### Bot Configuration Management”:
> Security note: Loader responses (list/get-config/compare) return redacted configurations — secrets like `botToken` and `clientSecret` are never included. Use actions to create/update secrets; loaders expose only non-sensitive fields.Also applies to: 238-252
slack/loaders/bot-routing/list.ts (1)
36-38: Guard against missing default bot.If
getDefaultBot()can be absent, this will throw andtotalConfigswill be wrong. Either enforce default existence at app init or makedefaultBotnullable here with a safe count.- const defaultBot = botRouter.getDefaultBot(); + const defaultBot = botRouter.getDefaultBot(); // verify it cannot be undefined at runtimeIf it can be undefined, change response type to allow
defaultBot?: PublicChannelBotConfigand computetotalConfigs = channelBots.length + (defaultBot ? 1 : 0).Also applies to: 49-55
slack/actions/bot-routing/remove-config.ts (1)
37-47: Consider idempotent delete semantics.Returning success when config is absent simplifies clients and aligns with common DELETE behavior.
- if (!botRouter.hasChannelBot(channelId)) { - return { - success: false, - message: `No bot configuration found for channel ${channelId}`, - }; - } + if (!botRouter.hasChannelBot(channelId)) { + return { + success: true, + message: `No configuration found for ${channelId}; nothing to remove.`, + }; + }Also applies to: 55-58
slack/actions/bot-routing/set-default.ts (1)
73-87: Consider validating optional credential fields.The validation only checks
botName,displayName, anddescription, but doesn't validate optional sensitive fields likebotToken,clientId,clientSecret, oravatarURL format. Invalid values in these fields could cause runtime errors when the bot attempts to authenticate or display its avatar.Consider adding validation for:
avatar: URL format validation if providedbotToken: non-empty and proper format (e.g., starts withxoxb-)clientId/clientSecret: non-empty if provided// Validate input const validation = validateBotConfig({ channelId: "*", // Wildcard for default botName, displayName, description, }); if (!validation.isValid) { return { success: false, message: "Validation failed", errors: validation.errors, }; } + + // Additional validation for optional fields + const additionalErrors: string[] = []; + if (botToken && !botToken.startsWith("xoxb-")) { + additionalErrors.push("Bot token must start with 'xoxb-'"); + } + if (avatar && !URL.canParse(avatar)) { + additionalErrors.push("Avatar must be a valid URL"); + } + if (additionalErrors.length > 0) { + return { + success: false, + message: "Validation failed", + errors: additionalErrors, + }; + }slack/utils/bot-router.ts (2)
63-68: Consider preservingcreatedAtinsetDefaultBotfor consistency.Similar to
setChannelBot, this method doesn't preservecreatedAtwhen updating. While callers likeset-default.tsexplicitly preserve it before calling this method (lines 95-110), the API would be more robust if it handled timestamp preservation internally.Apply this diff to make the API more defensive:
setDefaultBot(config: ChannelBotConfig): void { + const existing = this.config.defaultBot; this.config.defaultBot = { ...config, + createdAt: existing?.createdAt || config.createdAt || new Date().toISOString(), updatedAt: new Date().toISOString(), }; }
82-84: Consider returning a copy to prevent external mutation.
getDefaultBotreturns a direct reference to the internal default bot configuration, which allows external code to mutate it and bypasssetDefaultBot. For better encapsulation, return a shallow copy.Apply this diff:
getDefaultBot(): ChannelBotConfig { - return this.config.defaultBot; + return { ...this.config.defaultBot }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
slack/README.md(6 hunks)slack/actions/bot-routing/remove-config.ts(1 hunks)slack/actions/bot-routing/set-config.ts(1 hunks)slack/actions/bot-routing/set-default.ts(1 hunks)slack/actions/files/upload.ts(1 hunks)slack/actions/messages/post.ts(1 hunks)slack/actions/messages/react.ts(1 hunks)slack/actions/messages/threads/reply.ts(1 hunks)slack/loaders/bot-routing/compare.ts(1 hunks)slack/loaders/bot-routing/get-config.ts(1 hunks)slack/loaders/bot-routing/list.ts(1 hunks)slack/mod.ts(5 hunks)slack/types/bot-routing.ts(1 hunks)slack/utils/bot-router.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
slack/actions/bot-routing/remove-config.ts (3)
slack/actions/bot-routing/set-config.ts (1)
Props(5-60)slack/actions/bot-routing/set-default.ts (1)
Props(4-44)slack/mod.ts (2)
Props(59-118)AppContext(130-130)
slack/actions/bot-routing/set-config.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/utils/bot-router.ts (1)
validateBotConfig(160-190)
slack/loaders/bot-routing/list.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (1)
ChannelBotConfig(4-74)
slack/utils/bot-router.ts (1)
slack/types/bot-routing.ts (3)
BotRoutingConfig(79-94)ResolvedBotConfig(99-114)ChannelBotConfig(4-74)
slack/loaders/bot-routing/get-config.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (1)
ResolvedBotConfig(99-114)
slack/actions/bot-routing/set-default.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/utils/bot-router.ts (1)
validateBotConfig(160-190)
slack/loaders/bot-routing/compare.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (1)
ResolvedBotConfig(99-114)
slack/mod.ts (2)
slack/types/bot-routing.ts (1)
BotRoutingConfig(79-94)slack/utils/bot-router.ts (2)
BotRouter(6-126)createDefaultBotRouting(134-153)
🪛 LanguageTool
slack/README.md
[grammar] ~540-~540: Use a hyphen to join words.
Context: ...buting to this app: 1. Test both single bot and multi-bot routing scenarios 2. T...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
slack/README.md
456-456: Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (1)
slack/actions/bot-routing/set-default.ts (1)
94-110: Confirm that default bot should always be active.Line 105 hardcodes
isActive: true, which means the default bot cannot be deactivated through this action. If this is intentional (default must always be active), consider adding a comment to clarify the design decision.
This commit refactors Slack bot routing actions, loaders, types, and utilities for improved readability and consistent formatting. It also updates error messages to use template literals and fixes minor formatting issues in Google Drive and HubSpot utilities.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
slack/loaders/bot-routing/compare.ts (2)
11-20: Blocker: Loader exposes secrets (botToken, clientSecret). Redact before returning.The
ChannelComparisonResultinterface exposesResolvedBotConfigwhich includesChannelBotConfigcontaining sensitive fields (botToken,clientSecret). This creates a security vulnerability by leaking credentials to clients.Define a public shape that omits secrets and map responses to redacted configs:
-import { ResolvedBotConfig } from "../../types/bot-routing.ts"; +import { ResolvedBotConfig, ChannelBotConfig } from "../../types/bot-routing.ts"; + +type PublicChannelBotConfig = Omit<ChannelBotConfig, "botToken" | "clientSecret"> & { + hasBotToken: boolean; + hasClientSecret: boolean; +}; + +type PublicResolvedBotConfig = Omit<ResolvedBotConfig, "config"> & { + config: PublicChannelBotConfig; +}; + +const redact = (cfg: ChannelBotConfig): PublicChannelBotConfig => ({ + ...cfg, + botToken: undefined as never, + clientSecret: undefined as never, + hasBotToken: Boolean(cfg.botToken), + hasClientSecret: Boolean(cfg.clientSecret), +}); export interface ChannelComparisonResult { channelId: string; - resolvedConfig: ResolvedBotConfig; + resolvedConfig: PublicResolvedBotConfig; }
35-43: Apply redaction when mapping channel results.- const channels: ChannelComparisonResult[] = channelIds.map((channelId) => ({ - channelId, - resolvedConfig: botRouter.resolveForChannel(channelId), - })); + const channels: ChannelComparisonResult[] = channelIds.map((channelId) => { + const resolved = botRouter.resolveForChannel(channelId); + return { + channelId, + resolvedConfig: { ...resolved, config: redact(resolved.config) }, + }; + });slack/utils/bot-router.ts (2)
47-53: Preserve createdAt when updating channel bot.When updating an existing channel bot, the
createdAttimestamp is lost. This breaks audit trail capabilities and makes it impossible to track when configurations were originally created.setChannelBot(channelId: string, config: ChannelBotConfig): void { + const existing = this.config.channelBots[channelId]; this.config.channelBots[channelId] = { ...config, channelId, + createdAt: existing?.createdAt || config.createdAt || new Date().toISOString(), updatedAt: new Date().toISOString(), }; }
94-104: Inconsistent active status filtering.The method unconditionally includes
defaultBot(line 95) but only includes channel bots whenisActiveis true (line 98). This inconsistency could return inactive default bots.Apply consistent filtering:
listAllConfigurations(): ChannelBotConfig[] { - const configurations = [this.config.defaultBot]; + const configurations: ChannelBotConfig[] = []; + + if (this.config.defaultBot.isActive) { + configurations.push(this.config.defaultBot); + } Object.values(this.config.channelBots).forEach((config) => { if (config.isActive) { configurations.push(config); } }); return configurations; }slack/loaders/bot-routing/list.ts (2)
16-21: Blocker: Loader leaks secrets (botToken, clientSecret). Return redacted shape.The
BotConfigListResponseinterface returnsChannelBotConfigobjects that include sensitive fields (botToken,clientSecret). This exposes credentials to clients.Define and return a redacted DTO:
-import { ChannelBotConfig } from "../../types/bot-routing.ts"; +import { ChannelBotConfig } from "../../types/bot-routing.ts"; + +type PublicChannelBotConfig = Omit<ChannelBotConfig, "botToken" | "clientSecret"> & { + hasBotToken: boolean; + hasClientSecret: boolean; +}; + +const redact = (cfg: ChannelBotConfig): PublicChannelBotConfig => ({ + ...cfg, + botToken: undefined as never, + clientSecret: undefined as never, + hasBotToken: Boolean(cfg.botToken), + hasClientSecret: Boolean(cfg.clientSecret), +}); export interface BotConfigListResponse { - defaultBot: ChannelBotConfig; - channelBots: ChannelBotConfig[]; + defaultBot: PublicChannelBotConfig; + channelBots: PublicChannelBotConfig[]; totalConfigs: number; teamId: string; }
36-58: Apply redaction and sort results.Redact secrets before returning and sort by
updatedAtfor stable UX:const defaultBot = botRouter.getDefaultBot(); let channelBots: ChannelBotConfig[] = Object.values( botRouter.getAllChannelBots(), - ); + ).sort((a, b) => (b.updatedAt || "").localeCompare(a.updatedAt || "")); // Filter by channel if specified if (channelId) { channelBots = channelBots.filter((bot: ChannelBotConfig) => bot.channelId === channelId ); } // Filter out inactive bots unless requested if (!includeInactive) { channelBots = channelBots.filter((bot: ChannelBotConfig) => bot.isActive); } return { - defaultBot, - channelBots, + defaultBot: redact(defaultBot), + channelBots: channelBots.map(redact), totalConfigs: channelBots.length + 1, teamId: teamId || "", };
🧹 Nitpick comments (2)
slack/mod.ts (1)
207-219: Consider removing redundant botRouting from state.Since
botRouter.getConfig()returns the same data asbotRouting, storing both in state is redundant. Consider removing line 213 unless there's a specific serialization or access pattern requirement.Apply this diff if the redundancy is not needed:
const state: State = { ...appProps, slack: slackClientFor(appProps), slackClientFor, slackClientForChannel, botRouter, - botRouting, cb: { forTeam: (teamId: string, channel: string) => { return `${teamId}-${channel}`; }, }, };slack/loaders/bot-routing/compare.ts (1)
27-53: Add input validation for empty channelIds.Short-circuit execution when the input array is empty:
export default function compareChannelBotConfigs( props: Props, _req: Request, ctx: AppContext, ): BotConfigComparisonResponse { const { channelIds } = props; + + if (!Array.isArray(channelIds) || channelIds.length === 0) { + return { channels: [], uniqueConfigs: 0, allUsingDefault: true }; + } + const { botRouter } = ctx;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
google-drive/utils/query.ts(1 hunks)google-drive/utils/types.ts(3 hunks)hubspot/actions/conversations/archiveThread.ts(0 hunks)hubspot/actions/conversations/updateThread.ts(0 hunks)slack/actions/bot-routing/remove-config.ts(1 hunks)slack/actions/bot-routing/set-config.ts(1 hunks)slack/actions/bot-routing/set-default.ts(1 hunks)slack/actions/files/upload.ts(1 hunks)slack/actions/messages/post.ts(1 hunks)slack/actions/messages/react.ts(1 hunks)slack/actions/messages/threads/reply.ts(1 hunks)slack/loaders/bot-routing/compare.ts(1 hunks)slack/loaders/bot-routing/get-config.ts(1 hunks)slack/loaders/bot-routing/list.ts(1 hunks)slack/mod.ts(5 hunks)slack/types/bot-routing.ts(1 hunks)slack/utils/bot-router.ts(1 hunks)
💤 Files with no reviewable changes (2)
- hubspot/actions/conversations/updateThread.ts
- hubspot/actions/conversations/archiveThread.ts
✅ Files skipped from review due to trivial changes (2)
- google-drive/utils/query.ts
- google-drive/utils/types.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- slack/actions/files/upload.ts
- slack/actions/bot-routing/set-config.ts
- slack/loaders/bot-routing/get-config.ts
- slack/actions/bot-routing/set-default.ts
- slack/actions/bot-routing/remove-config.ts
- slack/types/bot-routing.ts
- slack/actions/messages/post.ts
🧰 Additional context used
🧬 Code graph analysis (4)
slack/loaders/bot-routing/compare.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (1)
ResolvedBotConfig(99-114)
slack/mod.ts (2)
slack/types/bot-routing.ts (1)
BotRoutingConfig(79-94)slack/utils/bot-router.ts (2)
BotRouter(10-130)createDefaultBotRouting(138-157)
slack/utils/bot-router.ts (1)
slack/types/bot-routing.ts (3)
BotRoutingConfig(79-94)ResolvedBotConfig(99-114)ChannelBotConfig(4-74)
slack/loaders/bot-routing/list.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (1)
ChannelBotConfig(4-74)
🔇 Additional comments (7)
slack/mod.ts (2)
143-149: LGTM: Safe default routing initialization.The initialization properly falls back to a default routing configuration when none is provided, ensuring the system always has valid bot routing.
190-205: LGTM: Channel-specific client creation with proper fallback.The implementation correctly resolves channel-specific bot configuration and falls back to app-level credentials when channel config doesn't override them. The credential hierarchy (channel-specific → app-level) ensures backward compatibility.
slack/utils/bot-router.ts (3)
22-40: Verify: Should resolveForChannel check defaultBot.isActive?When falling back to the default bot (lines 35-39), the method doesn't verify
defaultBot.isActive. If the default bot is deactivated, this could return an inactive configuration.Should the fallback check active status? If so:
// Fall back to default bot configuration + if (!this.config.defaultBot.isActive) { + throw new Error("Default bot is inactive and no active channel-specific bot found"); + } return { config: this.config.defaultBot, isDefault: true, channelId, };Alternatively, if an inactive default is acceptable, consider adding a comment explaining the design decision.
164-194: LGTM: Comprehensive validation logic.The validation function properly checks required fields, string lengths, and provides clear error messages. The constraints (80 chars for names, 250 for description) are reasonable.
138-157: LGTM: Clean default configuration factory.The function creates a well-structured default configuration with proper timestamps and sensible defaults (wildcard channelId, empty channelBots).
slack/actions/messages/react.ts (1)
31-36: LGTM: Correct channel-specific client usage.The implementation properly retrieves the channel-specific Slack client before adding the reaction, ensuring the correct bot configuration is used for the channel. The comment clearly explains the intent.
slack/actions/messages/threads/reply.ts (1)
32-37: LGTM: Correct channel-specific client usage.The implementation properly retrieves the channel-specific Slack client before posting the reply, ensuring the correct bot configuration is used for the channel. The comment is helpful and consistent with similar changes across the codebase.
Sensitive fields such as botToken and clientSecret are now omitted from API responses in bot config comparison and listing endpoints. New public config interfaces are introduced to represent redacted data, and presence flags are added to indicate if sensitive fields exist. The setChannelBot method now preserves createdAt and createdBy fields when updating configs. Documentation and code are updated for improved security and clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
slack/actions/bot-routing/set-config.ts (1)
109-144: LGTM! Bot ID and timestamps correctly preserved on updates.The implementation properly addresses the previous concerns about stable identity. The code now:
- Reuses the existing bot ID when updating (line 126)
- Preserves the original
createdAttimestamp (line 140)- Preserves the
createdByfield (line 142)- Uses proper type annotation for compile-time safety (line 129)
slack/utils/bot-router.ts (3)
47-57: LGTM!createdAtnow preserved correctly on channel bot updates.The implementation properly addresses the previous concern by preserving the original
createdAttimestamp when updating an existing channel bot (line 54). The fallback chain ensures a timestamp is always present.
98-114: LGTM! Consistent active status filtering implemented.The method now consistently filters both the default bot and channel-specific bots by their
isActivestatus (lines 102, 108). This addresses the previous concern about inconsistent filtering logic.
71-76: PreservecreatedAtwhen updating default bot.Unlike
setChannelBot(which correctly preservescreatedAt), this method overwrites the original creation timestamp on updates. The spread operator takescreatedAtfrom the incoming config, losing the existing value.Apply this diff to preserve
createdAtconsistently:setDefaultBot(config: ChannelBotConfig): void { + const existing = this.config.defaultBot; this.config.defaultBot = { ...config, + createdAt: existing?.createdAt || config.createdAt || new Date().toISOString(), updatedAt: new Date().toISOString(), }; }slack/loaders/bot-routing/compare.ts (3)
14-38: LGTM! Type-safe secret redaction implemented.The public interfaces correctly omit sensitive fields (
botToken,clientSecret) using TypeScript'sOmitutility type. This provides compile-time safety against accidentally exposing secrets in the API response.
74-93: LGTM! Runtime secret redaction correctly implemented.The helper functions properly strip sensitive fields at runtime using destructuring. The use of underscore-prefixed variables (
_botToken,_clientSecret) clearly indicates intentionally unused values.
64-70: LGTM! Appropriate guard for empty input.The early return for empty or undefined
channelIdsavoids unnecessary processing and provides sensible default values for the response.
🧹 Nitpick comments (1)
slack/README.md (1)
538-538: Consider hyphenating compound modifiers for consistency.The phrase "single bot and multi-bot" could use consistent hyphenation: "single-bot and multi-bot" per compound modifier rules.
Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
slack/README.md(6 hunks)slack/actions/bot-routing/set-config.ts(1 hunks)slack/loaders/bot-routing/compare.ts(1 hunks)slack/loaders/bot-routing/list.ts(1 hunks)slack/utils/bot-router.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- slack/loaders/bot-routing/list.ts
🧰 Additional context used
🧬 Code graph analysis (3)
slack/actions/bot-routing/set-config.ts (3)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/utils/bot-router.ts (1)
validateBotConfig(174-204)slack/types/bot-routing.ts (1)
ChannelBotConfig(4-74)
slack/loaders/bot-routing/compare.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (2)
ChannelBotConfig(4-74)ResolvedBotConfig(99-114)
slack/utils/bot-router.ts (1)
slack/types/bot-routing.ts (3)
BotRoutingConfig(79-94)ResolvedBotConfig(99-114)ChannelBotConfig(4-74)
🪛 LanguageTool
slack/README.md
[grammar] ~538-~538: Use a hyphen to join words.
Context: ...buting to this app: 1. Test both single bot and multi-bot routing scenarios 2. T...
(QB_NEW_EN_HYPHEN)
🔇 Additional comments (5)
slack/actions/bot-routing/set-config.ts (1)
146-162: LGTM! Configuration persistence and response handling are correct.The code properly persists the bot configuration to both the BotRouter and the app configuration, ensuring changes are saved. The success message appropriately indicates whether a configuration was created or updated.
slack/utils/bot-router.ts (2)
22-40: LGTM! Channel resolution logic is correct.The method properly resolves channel-specific configurations when active, falling back to the default bot configuration otherwise. The active status check ensures only enabled configurations are used.
121-123: LGTM! Correct existence and active status check.The method correctly checks both the existence of a channel-specific bot and its active status, consistent with the resolution logic elsewhere in the class.
slack/README.md (2)
18-59: LGTM! Comprehensive channel-specific bot routing documentation.The new documentation clearly explains the bot routing feature, including the routing architecture, configuration structure, and the ChannelBotConfig interface. The TypeScript interface example accurately reflects the actual implementation.
196-408: LGTM! Clear API documentation with practical examples.The bot routing API documentation is comprehensive and includes practical examples covering multiple use cases (support vs sales, multi-organization, department-specific bots, automatic resolution). The code examples accurately reflect the actual API surface.
Moved and unified public bot config interfaces to types/bot-routing.ts, ensuring sensitive fields are consistently omitted and presence flags are included where needed. Updated loaders to use the new types and redaction helpers. Improved setDefaultBot to preserve createdAt and update updatedAt.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
slack/types/bot-routing.ts (2)
125-141: Add hasClientId presence flag for parity and safer UX.Expose
hasClientIdinstead of the value; align with other flags.export interface PublicBotConfigWithFlags { id: string; channelId: string; botName: string; displayName?: string; avatar?: string; description?: string; isActive: boolean; createdAt: string; updatedAt: string; createdBy?: string; - metadata?: Record<string, unknown>; + // Optional safe subset; avoid returning raw metadata + publicMetadata?: Record<string, unknown>; // Presence flags for sensitive fields hasBotToken: boolean; hasClientSecret: boolean; + hasClientId: boolean; }
166-216: Mark sensitive fields and ensure they’re never logged.
botToken,clientSecret, and arguablyclientIdare sensitive inputs. Add JSDoc warnings and scrub from logs/metrics.Would you like a small guard util (e.g., redactForLogs) to centralize scrubbing before logging these payloads?
slack/loaders/bot-routing/list.ts (4)
36-38: Source teamId from router config for consistency.- const { botRouter, teamId } = ctx; + const { botRouter } = ctx; + const { teamId } = botRouter.getConfig();
68-71: Harden sort against bad/legacy timestamps.- channelBots.sort((a, b) => - new Date(b.updatedAt).getTime() - new Date(a.updatedAt).getTime() - ); + const toTime = (s: string) => { + const t = Date.parse(s); + return Number.isFinite(t) ? t : 0; + }; + channelBots.sort((a, b) => toTime(b.updatedAt) - toTime(a.updatedAt));
73-81: Apply includeInactive consistently and fix total count for inactive default.- const defaultBot = redactBotConfig(botRouter.getDefaultBot()); + const rawDefault = botRouter.getDefaultBot(); + const defaultBot = redactBotConfig(rawDefault); @@ - totalConfigs: redactedChannelBots.length + 1, // +1 for default bot + // Only count default if it's included by the filter + totalConfigs: + redactedChannelBots.length + + (includeInactive || rawDefault.isActive ? 1 : 0),If product intent is to always include default regardless of
includeInactive, ignore the count change but please document the behavior in the loader JSDoc.
39-49: If you add hasClientId/publicMetadata, update redaction here too.- const { botToken, clientSecret, ...publicFields } = config; + const { botToken, clientSecret, clientId, metadata, ...publicFields } = config; return { ...publicFields, hasBotToken: Boolean(botToken), hasClientSecret: Boolean(clientSecret), + hasClientId: Boolean(clientId), + publicMetadata: undefined, // or whitelist-copy from `metadata` };slack/loaders/bot-routing/compare.ts (2)
39-46: Deduplicate channelIds to avoid duplicate work and skewed stats.- const { channelIds } = props; + const { channelIds } = props; @@ - const channels: ChannelComparisonResult[] = channelIds.map((channelId) => ({ + const uniqueIds = Array.from(new Set(channelIds)); + const channels: ChannelComparisonResult[] = uniqueIds.map((channelId) => ({ channelId, resolvedConfig: redactResolvedConfig( botRouter.resolveForChannel(channelId), ), }));Also applies to: 71-76
50-60: Centralize redaction and, if policy changes, drop clientId/metadata.
- Consider importing a shared
redactBotConfigfrom a util.- If adopting stricter public types, remove
clientIdandmetadata, and add flags +publicMetadataas in list/get loaders.Also applies to: 62-69
slack/utils/bot-router.ts (1)
75-80: Enforce default channelId “*” and stable id when setting default.setDefaultBot(config: ChannelBotConfig): void { const existingDefaultBot = this.config.defaultBot; const now = new Date().toISOString(); this.config.defaultBot = { ...config, + // Default applies globally + channelId: "*", + // Preserve a stable id or provide one if missing + id: config.id || existingDefaultBot?.id || `default-${this.config.teamId}`, createdAt: existingDefaultBot?.createdAt || config.createdAt || now, updatedAt: now, }; }slack/loaders/bot-routing/get-config.ts (1)
21-52: Redaction verified correct; centralize helper functions to reduce duplication.The code properly redacts sensitive fields and returns
PublicResolvedBotConfig. Verification confirms bothget-config.tsandcompare.tsapply redaction before returning, so no loaders expose the internalResolvedBotConfigtype.To eliminate code duplication, extract the
redactBotConfigandredactResolvedConfighelpers into a shared utility (e.g.,slack/utils/redact.ts) and import in both loaders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
slack/loaders/bot-routing/compare.ts(1 hunks)slack/loaders/bot-routing/get-config.ts(1 hunks)slack/loaders/bot-routing/list.ts(1 hunks)slack/types/bot-routing.ts(1 hunks)slack/utils/bot-router.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
slack/utils/bot-router.ts (1)
slack/types/bot-routing.ts (3)
BotRoutingConfig(79-94)ResolvedBotConfig(99-114)ChannelBotConfig(4-74)
slack/loaders/bot-routing/compare.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (4)
PublicResolvedBotConfig(146-161)ChannelBotConfig(4-74)PublicChannelBotConfig(119-120)ResolvedBotConfig(99-114)
slack/loaders/bot-routing/get-config.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (4)
PublicResolvedBotConfig(146-161)ChannelBotConfig(4-74)PublicChannelBotConfig(119-120)ResolvedBotConfig(99-114)
slack/loaders/bot-routing/list.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (2)
PublicBotConfigWithFlags(125-141)ChannelBotConfig(4-74)
Redacts sensitive fields (clientId, metadata) from public bot configs and exposes presence flags and a safe publicMetadata subset. Updates types to reflect new fields and ensures only safe metadata values are included. Adds error handling for missing or inactive default bot configs and returns a deep copy in getConfig.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
slack/loaders/bot-routing/list.ts (2)
88-91: Harden sort against invalid timestamps.new Date(s).getTime() can be NaN, yielding unstable ordering. Fall back to createdAt and 0.
- channelBots.sort((a, b) => - new Date(b.updatedAt).getTime() - new Date(a.updatedAt).getTime() - ); + const ts = (s?: string) => { + const t = s ? Date.parse(s) : NaN; + return Number.isNaN(t) ? 0 : t; + }; + channelBots.sort((a, b) => { + const tb = ts(b.updatedAt ?? b.createdAt); + const ta = ts(a.updatedAt ?? a.createdAt); + return tb - ta; + });
83-86: Clarify includeInactive semantics for default and total.Default is always returned even when includeInactive is false, while channel bots are filtered. Either: (a) keep always including default but document this invariant in Props; or (b) filter default similarly and make totalConfigs reflect actual returned count. Pick one for consistency.
Also applies to: 96-101
slack/loaders/bot-routing/compare.ts (2)
37-38: Deduplicate channelIds before processing.Prevents duplicate entries and stabilizes uniqueConfigs/allUsingDefault.
- const { channelIds } = props; + const { channelIds } = props; + const uniqIds = Array.from(new Set((channelIds ?? []).filter(Boolean))); @@ - const channels: ChannelComparisonResult[] = channelIds.map((channelId) => ({ + const channels: ChannelComparisonResult[] = uniqIds.map((channelId) => ({ @@ - const uniqueConfigIds = new Set( - channels.map((ch) => ch.resolvedConfig.config.id), - ); + const uniqueConfigIds = new Set(channels.map((ch) => ch.resolvedConfig.config.id));Also applies to: 94-99, 101-113
50-83: Avoid redaction duplication across loaders.Both compare.ts and list.ts implement similar redact logic. Extract a shared helper (e.g., slack/utils/public-bot-config.ts) to prevent drift.
slack/utils/bot-router.ts (3)
98-100: Return deep clone to avoid external mutation of channelBots.Shallow copy allows nested mutation. Use structuredClone.
- getAllChannelBots(): Record<string, ChannelBotConfig> { - return { ...this.config.channelBots }; - } + getAllChannelBots(): Record<string, ChannelBotConfig> { + return structuredClone(this.config.channelBots); + }
106-108: Return a clone for default bot as well.Prevents callers from mutating internal state via reference.
- getDefaultBot(): ChannelBotConfig { - return this.config.defaultBot; - } + getDefaultBot(): ChannelBotConfig { + return structuredClone(this.config.defaultBot); + }
153-155: Consider validating on updateConfig.Optionally run validateBotConfig on default and each channel entry and/or enforce active default here. Keeps invariants centralized.
slack/types/bot-routing.ts (2)
116-129: Unify public types to reduce duplication and drift.Derive PublicBotConfigWithFlags from PublicChannelBotConfig to keep a single source of truth.
-export interface PublicBotConfigWithFlags { - id: string; - channelId: string; - botName: string; - displayName?: string; - avatar?: string; - description?: string; - isActive: boolean; - createdAt: string; - updatedAt: string; - createdBy?: string; - - // Curated public metadata (safe subset) - publicMetadata?: Record<string, string | number | boolean>; - - // Presence flags for sensitive fields - hasBotToken: boolean; - hasClientSecret: boolean; - hasClientId: boolean; - hasMetadata: boolean; -} +export type PublicBotConfigWithFlags = + PublicChannelBotConfig & { + hasBotToken: boolean; + hasClientSecret: boolean; + };Also applies to: 134-154
1-74: Public response types could be readonly.Mark fields readonly to communicate immutability at the API boundary. Optional polish.
Also applies to: 116-174
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
slack/loaders/bot-routing/compare.ts(1 hunks)slack/loaders/bot-routing/get-config.ts(1 hunks)slack/loaders/bot-routing/list.ts(1 hunks)slack/types/bot-routing.ts(1 hunks)slack/utils/bot-router.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- slack/loaders/bot-routing/get-config.ts
🧰 Additional context used
🧬 Code graph analysis (3)
slack/loaders/bot-routing/list.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (2)
PublicBotConfigWithFlags(134-154)ChannelBotConfig(4-74)
slack/utils/bot-router.ts (1)
slack/types/bot-routing.ts (3)
BotRoutingConfig(79-94)ResolvedBotConfig(99-114)ChannelBotConfig(4-74)
slack/loaders/bot-routing/compare.ts (2)
slack/mod.ts (2)
Props(59-118)AppContext(130-130)slack/types/bot-routing.ts (4)
PublicResolvedBotConfig(159-174)ChannelBotConfig(4-74)PublicChannelBotConfig(119-129)ResolvedBotConfig(99-114)
🔇 Additional comments (5)
slack/loaders/bot-routing/list.ts (1)
39-69: Redaction looks solid.Secrets are stripped and presence flags + curated publicMetadata are returned. This addresses prior leakage concerns.
slack/loaders/bot-routing/compare.ts (3)
39-46: Good early return for empty input.Avoids unnecessary work and clearly defines response.
50-83: Redaction and curated metadata are correct.Secrets are omitted; only primitive metadata is surfaced with presence flags.
94-99: Resolution errors would fail the whole request.BotRouter.resolveForChannel can throw when default is missing/inactive. Confirm invariant: app init guarantees an active default, or handle per-channel failures (would require API change).
slack/utils/bot-router.ts (1)
145-147: structuredClone runtime support.If this may run outside Deno/modern Node, add a fallback to JSON round‑trip or a tiny polyfill. If Deno‑only, ignore.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
slack/utils/bot-router.ts (2)
98-100: Consider returning a deep copy for consistency.This method returns a shallow copy of
channelBots, which means callers can still mutate the nestedChannelBotConfigobjects. This is inconsistent withgetConfig()(lines 145-150), which returns a deep clone to prevent external mutation.Apply this diff to align with the immutability pattern established by
getConfig():getAllChannelBots(): Record<string, ChannelBotConfig> { - return { ...this.config.channelBots }; + return typeof structuredClone === "function" + ? structuredClone(this.config.channelBots) + : JSON.parse(JSON.stringify(this.config.channelBots)); }
114-130: Active-only filtering correctly implemented.The method now properly filters to include only active configurations for both default and channel-specific bots, addressing the previous inconsistency concern.
Note: The returned array contains direct references to the configuration objects. For full immutability, consider deep-cloning the configs before pushing them to the array, similar to the approach in
getConfig()(lines 145-150).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
slack/utils/bot-router.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
slack/utils/bot-router.ts (1)
slack/types/bot-routing.ts (3)
BotRoutingConfig(79-94)ResolvedBotConfig(99-114)ChannelBotConfig(4-74)
🔇 Additional comments (6)
slack/utils/bot-router.ts (6)
22-52: LGTM! Default bot validation properly enforced.The resolution logic correctly prioritizes active channel-specific bots, then validates the default bot's existence and active status before fallback. The fail-fast approach when no active default is available prevents silent failures downstream.
59-69: LGTM! Timestamp preservation implemented correctly.The method properly preserves the original
createdAttimestamp when updating an existing channel bot while correctly updatingupdatedAt.
83-92: LGTM! Default bot update preserves creation timestamp.The method correctly preserves the original
createdAtwhen updating the default bot configuration.
145-150: LGTM! Deep cloning properly implemented.The method correctly returns a deep copy of the routing configuration using
structuredClonewhen available, with an appropriate JSON round-trip fallback. This prevents external mutation of the router's internal state.
167-186: LGTM! Default routing initialization is correct.The function creates a properly structured default routing configuration with sensible defaults. The use of
"*"as a wildcardchannelIdfor the default bot is a clear convention.
193-223: LGTM! Validation covers essential constraints.The validation function correctly checks required fields and enforces length limits aligned with Slack's platform constraints.
Replaces direct assignment and return of bot configuration objects with structuredClone to ensure deep copies are used. This prevents unintended mutations of the original configuration.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
slack/utils/bot-router.ts (1)
193-223: Consider expanding validation coverage.While the current validation covers the most critical user-facing fields, consider adding validation for:
idformat (e.g., non-empty, valid characters)isActivepresence when creating new configs- Token/credential formats if provided
This is acceptable for the current scope but could improve robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
slack/utils/bot-router.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
slack/utils/bot-router.ts (1)
slack/types/bot-routing.ts (3)
BotRoutingConfig(79-94)ResolvedBotConfig(99-114)ChannelBotConfig(4-74)
🔇 Additional comments (3)
slack/utils/bot-router.ts (3)
22-52: LGTM! Proper validation of default bot.The fallback logic now correctly validates that the default bot exists and is active before using it, preventing the use of inactive configurations.
59-69: LGTM! Timestamp handling is correct.The method now properly preserves the existing
createdAttimestamp when updating a channel bot configuration, while correctly updatingupdatedAt.
83-92: LGTM! Timestamp preservation implemented correctly.The method now properly preserves the existing
createdAttimestamp when updating the default bot, consistent withsetChannelBot.
Replaces direct assignment of the config object with structuredClone to prevent unintended side effects from external mutations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
slack/utils/bot-router.ts (2)
98-100: Return a deep copy to prevent mutation of internal state.This method still returns a shallow copy, exposing direct references to internal
ChannelBotConfigobjects. Callers can mutate these objects and break the router's state integrity. This is inconsistent withgetDefaultBot(line 107) andgetConfig(lines 145-150), which return deep clones.Apply this diff to protect internal state:
getAllChannelBots(): Record<string, ChannelBotConfig> { - return { ...this.config.channelBots }; + return structuredClone(this.config.channelBots); }
114-130: Return deep copies to prevent mutation of internal state.The method pushes direct references to internal configuration objects into the returned array. Callers can mutate these objects and break the router's state integrity. This is inconsistent with
getDefaultBotandgetConfig, which return deep clones.Apply this diff to protect internal state:
listAllConfigurations(): ChannelBotConfig[] { const configurations: ChannelBotConfig[] = []; // Only include default bot if it exists and is active if (this.config.defaultBot && this.config.defaultBot.isActive) { - configurations.push(this.config.defaultBot); + configurations.push(structuredClone(this.config.defaultBot)); } // Include channel bots that are active Object.values(this.config.channelBots).forEach((config) => { if (config.isActive) { - configurations.push(config); + configurations.push(structuredClone(config)); } }); return configurations; }
🧹 Nitpick comments (2)
slack/utils/bot-router.ts (2)
137-139: Consider renaming to clarify that this checks active status, not just existence.The method name
hasChannelBotsuggests it checks whether a channel-specific bot configuration exists, but the implementation only returnstruewhen the bot both exists AND is active (isActive === true). This could confuse callers who expect a simple existence check.Consider one of these options:
Option 1 (recommended): Rename to clarify intent:
- hasChannelBot(channelId: string): boolean { + hasActiveChannelBot(channelId: string): boolean { return !!this.config.channelBots[channelId]?.isActive; }Option 2: Split into two methods:
hasChannelBot(channelId: string): boolean { return !!this.config.channelBots[channelId]; } isChannelBotActive(channelId: string): boolean { return !!this.config.channelBots[channelId]?.isActive; }
193-223: Consider validating additional fields for robustness.The current validation covers the essential fields (
botName,channelId) and length constraints, but could be enhanced to validate:
isActiveshould be a boolean (if provided)idshould be a non-empty string (if provided)botToken,clientId,clientSecretformat/presence consistency (e.g., ifclientIdis provided,clientSecretshould also be provided)Apply this diff to enhance validation:
const errors: string[] = []; if (!config.botName || config.botName.trim() === "") { errors.push("Bot name is required"); } if (!config.channelId || config.channelId.trim() === "") { errors.push("Channel ID is required"); } if (config.botName && config.botName.length > 80) { errors.push("Bot name must be 80 characters or less"); } if (config.displayName && config.displayName.length > 80) { errors.push("Display name must be 80 characters or less"); } if (config.description && config.description.length > 250) { errors.push("Description must be 250 characters or less"); } + if (config.isActive !== undefined && typeof config.isActive !== "boolean") { + errors.push("isActive must be a boolean"); + } + + if (config.id !== undefined && (!config.id || config.id.trim() === "")) { + errors.push("id must be a non-empty string"); + } + + // Validate OAuth credentials consistency + if ((config.clientId && !config.clientSecret) || (!config.clientId && config.clientSecret)) { + errors.push("clientId and clientSecret must be provided together"); + } return { isValid: errors.length === 0, errors, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
slack/utils/bot-router.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
slack/utils/bot-router.ts (1)
slack/types/bot-routing.ts (3)
BotRoutingConfig(79-94)ResolvedBotConfig(99-114)ChannelBotConfig(4-74)
🔇 Additional comments (4)
slack/utils/bot-router.ts (4)
13-52: LGTM! Constructor and resolution logic properly addressed previous concerns.The constructor now defensively clones the config, and
resolveForChannelcorrectly validates that the default bot exists and is active before falling back to it.
59-92: LGTM! Mutation methods correctly preserve timestamps.All setter methods now properly preserve
createdAtwhen updating existing configurations while correctly updatingupdatedAt.
145-158: LGTM! Configuration getters and setters properly implement defensive cloning.Both methods correctly use
structuredCloneto prevent external mutation of internal state, with appropriate fallback for environments without nativestructuredClonesupport.
167-186: LGTM! Default routing configuration is well-structured.The function creates a sensible default configuration with proper timestamps and a wildcard channel ID for the default bot.
Updated getAllChannelBots to return a deep copy of channel bot configurations using structuredClone instead of a shallow copy. This prevents unintended mutations to the original configuration object.
Replaces direct references with structured clones when adding default and channel bot configurations to the list. This prevents unintended side effects from shared object references.
Renames hasChannelBot to hasActiveChannelBot for clarity and updates its usage in set-config and remove-config actions. Enhances validateBotConfig to check isActive type, non-empty id, and consistency of OAuth credentials.
Introduces channel-specific bot routing, allowing configuration of different bots per channel. Adds bot routing actions, loaders, types, and utilities. Updates Slack actions to use channel-resolved bot clients, and enhances documentation with usage, migration, and troubleshooting details.
Summary by CodeRabbit
New Features
Documentation
Refactor
Style/Utilities
Chores