-
Notifications
You must be signed in to change notification settings - Fork 15
Hotfix(Slack): Improve Slack Custom Bot Flow #1420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…x/apps-mcp into feat/slack-custom-bot
…eat/slack-custom-bot
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tart loader for custom bot handling, and improve UI for bot selection page
…x/apps-mcp into feat/slack-custom-bot
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughReworks Slack OAuth flows: callback becomes stateful and permission-aware (handles savePermission/continue paths, validates and stores permissions, or proceeds to token exchange and workspace/user/channel fetch), adds permission types and a selection UI generator/template, and simplifies the OAuth start loader to always return a redirect to Slack authorization URL. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant B as Browser
participant S as App Server (callback)
participant SL as Slack API
Note over S: Request includes code + state (+ optional savePermission/continue/permissions)
B->>S: GET /oauth/callback?code&state(&savePermission|continue|permissions)
S->>S: decodeState(state)
alt savePermission path
S->>S: decode & validate permissions payload
S->>S: hasExistingPermissions? -> maybe createPermissionExistsError
S-->>B: JSON { installId, account?, permission? } or error
else continue or initial presentation
S->>SL: oauth.v2.access(code)
SL-->>S: OAuthTokenResponse (access_token, authed_user, team, bot, ...)
S->>SL: team.info / users.info / conversations.list (using tokens)
SL-->>S: workspace, user, channels
S->>S: generateSelectionPage(workspace, channels, user, callbackUrl)
S-->>B: 200 HTML (selection UI)
end
sequenceDiagram
autonumber
participant Client as Caller
participant S as App Server (start)
Note over S: New simplified start(props) -> redirect
Client->>S: start(props: { clientId, redirectUri, state })
S->>S: build authParams (scopes, client_id, redirect_uri, state)
S-->>Client: 302 -> https://slack.com/oauth/v2/authorize?...state=...
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
🧹 Nitpick comments (2)
slack/actions/oauth/callback.ts (2)
100-105: Improve type safety in permission check.The
hasExistingPermissionsfunction uses a loose type check andObject.keys()that could yield false positives. An empty object would pass the check even though it has no meaningful permissions.Consider a more specific check:
function hasExistingPermissions(permission: unknown): boolean { if (!permission || typeof permission !== "object" || permission === null) { return false; } - return Object.keys(permission as Record<string, unknown>).length > 0; + const p = permission as { workspace?: unknown; channels?: unknown; allCurrentAndFutureChannels?: boolean }; + return !!(p.workspace || p.channels || p.allCurrentAndFutureChannels); }
266-268: Document the 50-channel limit.The hardcoded limit of 50 channels (line 266) is mentioned in comments and the UI ("Showing first 50 channels"), but the business logic rationale isn't documented. Consider whether pagination or a configurable limit would better serve workspaces with many channels.
Add a constant with documentation:
const MAX_CHANNELS_FOR_DISPLAY = 50; // Fetch channels (limit to first 50 for display) const channelsResponse = await fetch( "https://slack.com/api/conversations.list", { method: "POST", headers: { "Authorization": `Bearer ${tokenData.access_token}`, "Content-Type": "application/x-www-form-urlencoded", }, body: new URLSearchParams({ limit: MAX_CHANNELS_FOR_DISPLAY.toString(), types: "public_channel,private_channel", }), }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
slack/actions/oauth/callback.ts(1 hunks)slack/mod.ts(2 hunks)slack/utils/ui-templates/page-generator.ts(7 hunks)slack/utils/ui-templates/selection-page.template.html(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
slack/actions/oauth/callback.ts (4)
slack/mod.ts (3)
Props(57-123)State(125-131)AppContext(133-133)slack/loaders/oauth/start.ts (1)
Props(10-19)slack/utils/state-helpers.ts (1)
decodeState(6-13)slack/utils/ui-templates/page-generator.ts (1)
generateSelectionPage(31-703)
slack/mod.ts (1)
mcp/bindings.ts (1)
Callbacks(9-13)
🪛 GitHub Actions: ci
slack/utils/ui-templates/selection-page.template.html
[error] 471-632: Deno fmt --check failed. Found 1 not formatted file in 2077 files. Run 'deno fmt' to fix formatting issues in this file.
🔇 Additional comments (3)
slack/mod.ts (1)
44-55: LGTM!The
SlackPermissioninterface is well-structured and clearly defines the permission model for workspace and channel access.slack/actions/oauth/callback.ts (2)
4-24: LGTM!The
OAuthTokenResponseinterface accurately models the Slack OAuth v2 token response structure.
288-296: No changes needed for callback URL
URLSearchParams.set("savePermission", "true")retains all existing query parameters and only overrides the specified key.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
slack/loaders/oauth/start.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
slack/loaders/oauth/start.ts (3)
slack/mod.ts (1)
Props(57-110)slack/actions/oauth/callback.ts (1)
Props(26-69)slack/utils/constants.ts (2)
SCOPES(1-17)OAUTH_URL_AUTH(21-21)
…tor.ts to match original implementation
- Add comprehensive validation for base64 permissions data in callback.ts - Implement proper error handling for parallel Slack API fetches - Fix Unicode-safe base64 encoding in page-generator.ts - Correct workspace domain display in selection-page.template.html - Clean up fallback page HTML structure - Run deno fmt on selection-page.template.html for proper formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
slack/actions/oauth/callback.ts(1 hunks)slack/utils/ui-templates/page-generator.ts(1 hunks)slack/utils/ui-templates/selection-page.template.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- slack/utils/ui-templates/selection-page.template.html
🧰 Additional context used
🧬 Code graph analysis (1)
slack/actions/oauth/callback.ts (4)
slack/loaders/oauth/start.ts (1)
Props(3-7)slack/mod.ts (3)
Props(57-110)State(112-118)AppContext(120-120)slack/utils/state-helpers.ts (1)
decodeState(6-13)slack/utils/ui-templates/page-generator.ts (1)
generateSelectionPage(31-715)
…ction page template
…generateSelectionPage function
Summary by CodeRabbit
New Features
Improvements