Fix alert creation bug related to the Slack integration #1967
Conversation
|
WalkthroughThe changes implement robust error handling for Slack integration within the alert channel setup flow. The Slack channel fetch logic now detects revoked tokens and other Slack API errors, returning specific statuses such as "TOKEN_REVOKED," "TOKEN_EXPIRED," and "FAILED_FETCHING_CHANNELS." The UI is updated to recognize these new statuses, displaying appropriate callouts and offering a Slack reconnection option if access is revoked or expired. Additionally, the Slack connection route now supports a "reinstall" query parameter to facilitate reconnection. Unused imports are removed from relevant files for clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Server
participant Slack
User->>UI: Open New Alert Modal
UI->>Server: Fetch Slack Channels
Server->>Slack: API call to fetch channels
alt Token revoked or expired
Slack-->>Server: Error (token_revoked or token_expired)
Server-->>UI: Status "TOKEN_REVOKED" or "TOKEN_EXPIRED"
UI-->>User: Show callout, offer reconnect button
User->>UI: Click "Connect to Slack"
UI->>Server: Redirect to Slack auth with reinstall param
else Other Slack error
Slack-->>Server: Error (other)
Server-->>UI: Status "FAILED_FETCHING_CHANNELS"
UI-->>User: Show warning callout
else Success
Slack-->>Server: Channels list
Server-->>UI: Show channels
end
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/webapp/app/presenters/v3/NewAlertChannelPresenter.server.ts (1)
120-131:isSlackErrorcan be simplified & made type‑safer
- Use a type predicate that narrows on
SlackAPIErrorrather than a bespoke shape to prevent false positives.- Rely on
inoperator only once after confirming the object is notnull.-function isSlackError(obj: unknown): obj is { data: { error: string } } { - return Boolean( - typeof obj === "object" && - obj !== null && - "data" in obj && - typeof obj.data === "object" && - obj.data !== null && - "error" in obj.data && - typeof obj.data.error === "string" - ); +interface SlackAPIError { + data: { error: string }; +} + +function isSlackError(obj: unknown): obj is SlackAPIError { + if (typeof obj !== "object" || obj === null) return false; + const maybe = obj as Record<string, unknown>; + const data = maybe["data"]; + return ( + typeof data === "object" && + data !== null && + typeof (data as Record<string, unknown>)["error"] === "string" + ); }This keeps the guard concise and easier to maintain.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new.connect-to-slack.ts (2)
34-43: Success message may confuse users when no (re)installation occurredWhen an integration already exists and
reinstallis not requested, the loader redirects with
"Successfully connected your Slack workspace"even though no OAuth round‑trip happened in this request.
Consider wording such as “Slack workspace already connected” to avoid misleading feedback.
45-53: Avoid awaiting a function that already returns aResponse
OrgIntegrationRepository.redirectToAuthService(...)already returns aResponse.
The surroundingawaitis unnecessary and can be dropped, saving a tick and making intent clearer.- return await OrgIntegrationRepository.redirectToAuthService( + return OrgIntegrationRepository.redirectToAuthService( "SLACK", project.organizationId, request, v3NewProjectAlertPathConnectToSlackPath({ slug: organizationSlug }, project, { slug: envParam, }) );apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx (2)
359-378: Maintain layout consistency by re‑usingInputGroupwrapperFor the
"ACCESS_REVOKED"branch we fall back to a raw<div>whereas the other branches stay inside anInputGroup.
This can lead to inconsistent spacing/margins compared to the rest of the form.- ) : slack.status === "ACCESS_REVOKED" ? ( - <div className="flex flex-col gap-4"> + ) : slack.status === "ACCESS_REVOKED" ? ( + <InputGroup fullWidth className="flex flex-col gap-4"> ... - </div> + </InputGroup>Keeps vertical rhythm identical across all states.
379-383: Allow quick retry after transient Slack errorsIn the
"FAILED_FETCHING_CHANNELS"state we only show a Callout without any action.
Providing a lightweight “Retry” button could improve UX for transient outages.) : slack.status === "FAILED_FETCHING_CHANNELS" ? ( <div className="flex flex-col gap-4"> <Callout variant="warning"> Failed loading channels from Slack. Please try again later. </Callout> + <Button + variant="tertiary/medium" + onClick={() => navigate(0)} // full page reload + > + Retry + </Button> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/app/presenters/v3/NewAlertChannelPresenter.server.ts(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new.connect-to-slack.ts(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx(1 hunks)apps/webapp/app/v3/services/createOrgIntegration.server.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/webapp/app/v3/services/createOrgIntegration.server.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/webapp/app/presenters/v3/NewAlertChannelPresenter.server.ts (1)
apps/webapp/app/models/orgIntegration.server.ts (1)
OrgIntegrationRepository(50-244)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
| const [error, channels] = await tryCatch(getSlackChannelsForToken(slackIntegration)); | ||
|
|
||
| if (error) { | ||
| if (isSlackError(error) && error.data.error === "token_revoked") { | ||
| return { | ||
| slack: { | ||
| status: "ACCESS_REVOKED" as const, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| logger.error("Failed fetching Slack channels for creating alerts", { | ||
| error, | ||
| slackIntegrationId: slackIntegration.id, | ||
| }); | ||
|
|
||
| return { | ||
| slack: { | ||
| status: "FAILED_FETCHING_CHANNELS" as const, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
token_revoked scenario will never be detected with the current error propagation
getAllSlackConversations() converts any Slack‑API error into a plain Error (see lines 110‑113) thereby discarding the original data.error payload that isSlackError() relies on.
As a result, the "ACCESS_REVOKED" path is unreachable and users with a revoked token will instead fall into "FAILED_FETCHING_CHANNELS".
- if (!response.ok) {
- throw new Error(`Failed to get channels: ${response.error}`);
+ if (!response.ok) {
+ // Preserve Slack error semantics so the presenter can react accurately
+ const err: { data: { error: string } } = {
+ data: { error: response.error ?? "unknown_error" },
+ };
+ throw err;
}Alternatively, parse error.message when it contains "token_revoked" inside the presenter, but preserving the structured object keeps concerns better separated.
Committable suggestion skipped: line range outside the PR's diff.
2 participants
This PR fixes a bug which broke the creation of new alerts when an existing Slack integration is expired/revoked by the user. Also, transient errors when listing slack channels are now handled gracefully.
An approach to handle expired tokens automatically might still be needed; remains for a separate PR.
Closes #1414
Screenshots
A note about the token revoked issue is now shown the user alongside a button to reconnect the slack integration.
Summary by CodeRabbit
New Features
Bug Fixes
Chores