Skip to content

Fix alert creation bug related to the Slack integration #1967

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 23, 2025
Merged

Fix alert creation bug related to the Slack integration #1967

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8f4fba
Fix alert creation bug related to the Slack integration
myftija Apr 22, 2025
24d23c9
Handle token_expired error from slack gracefully
myftija Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 53 additions & 24 deletions apps/webapp/app/presenters/v3/NewAlertChannelPresenter.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import {
AuthenticatableIntegration,
type AuthenticatableIntegration,
OrgIntegrationRepository,
} from "~/models/orgIntegration.server";
import { logger } from "~/services/logger.server";
import { BasePresenter } from "./basePresenter.server";
import { WebClient } from "@slack/web-api";
import { type WebClient } from "@slack/web-api";
import { tryCatch } from "@trigger.dev/core";
import { logger } from "~/services/logger.server";

export class NewAlertChannelPresenter extends BasePresenter {
public async call(projectId: string) {
Expand All @@ -30,42 +31,58 @@ export class NewAlertChannelPresenter extends BasePresenter {

// If there is a slack integration, then we need to get a list of Slack Channels
if (slackIntegration) {
const channels = await getSlackChannelsForToken(slackIntegration);
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,
},
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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.


return {
slack: {
status: "READY" as const,
channels,
channels: channels ?? [],
integrationId: slackIntegration.id,
},
};
} else {
if (OrgIntegrationRepository.isSlackSupported) {
return {
slack: {
status: "NOT_CONFIGURED" as const,
},
};
} else {
return {
slack: {
status: "NOT_AVAILABLE" as const,
},
};
}
}

if (OrgIntegrationRepository.isSlackSupported) {
return {
slack: {
status: "NOT_CONFIGURED" as const,
},
};
}

return {
slack: {
status: "NOT_AVAILABLE" as const,
},
};
}
}

async function getSlackChannelsForToken(integration: AuthenticatableIntegration) {
const client = await OrgIntegrationRepository.getAuthenticatedClientForIntegration(integration);

const channels = await getAllSlackConversations(client);

logger.debug("Received a list of slack conversations", {
channels,
});

return (channels ?? [])
.filter((channel) => !channel.is_archived)
.filter((channel) => channel.is_channel)
Expand Down Expand Up @@ -100,3 +117,15 @@ async function getAllSlackConversations(client: WebClient) {

return channels;
}

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"
);
}
28 changes: 15 additions & 13 deletions ...rgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new.connect-to-slack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import { type LoaderFunctionArgs } from "@remix-run/server-runtime";
import { prisma } from "~/db.server";
import { env } from "~/env.server";
import { redirectWithSuccessMessage } from "~/models/message.server";
import { OrgIntegrationRepository } from "~/models/orgIntegration.server";
import { findProjectBySlug } from "~/models/project.server";
import { requireUserId } from "~/services/session.server";
import {
EnvironmentParamSchema,
ProjectParamSchema,
v3NewProjectAlertPath,
v3NewProjectAlertPathConnectToSlackPath,
} from "~/utils/pathBuilder";
Expand All @@ -16,6 +14,9 @@ export async function loader({ request, params }: LoaderFunctionArgs) {
const userId = await requireUserId(request);
const { organizationSlug, projectParam, envParam } = EnvironmentParamSchema.parse(params);

const url = new URL(request.url);
const shouldReinstall = url.searchParams.get("reinstall") === "true";

const project = await findProjectBySlug(organizationSlug, projectParam, userId);

if (!project) {
Expand All @@ -30,23 +31,24 @@ export async function loader({ request, params }: LoaderFunctionArgs) {
},
});

if (integration) {
// If integration exists and we're not reinstalling, redirect back to alerts
if (integration && !shouldReinstall) {
return redirectWithSuccessMessage(
`${v3NewProjectAlertPath({ slug: organizationSlug }, project, {
slug: envParam,
})}?option=slack`,
request,
"Successfully connected your Slack workspace"
);
} else {
// Redirect to Slack
return await OrgIntegrationRepository.redirectToAuthService(
"SLACK",
project.organizationId,
request,
v3NewProjectAlertPathConnectToSlackPath({ slug: organizationSlug }, project, {
slug: envParam,
})
);
}

// Redirect to Slack for new installation or reinstallation
return await OrgIntegrationRepository.redirectToAuthService(
"SLACK",
project.organizationId,
request,
v3NewProjectAlertPathConnectToSlackPath({ slug: organizationSlug }, project, {
slug: envParam,
})
);
}
25 changes: 25 additions & 0 deletions ...tes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new/route.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,31 @@ export default function Page() {
<SlackIcon className="size-5" /> Connect to Slack
</span>
</LinkButton>
) : slack.status === "ACCESS_REVOKED" ? (
<div className="flex flex-col gap-4">
<Callout variant="info">
The Slack integration in your workspace has been revoked. Please re-connect
your Slack workspace.
</Callout>
<LinkButton
variant="tertiary/large"
to={{
pathname: "connect-to-slack",
search: "?reinstall=true",
}}
fullWidth
>
<span className="flex items-center gap-2 text-text-bright">
<SlackIcon className="size-5" /> Connect to Slack
</span>
</LinkButton>
</div>
) : 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>
</div>
) : (
<Callout variant="warning">
Slack integration is not available. Please contact your organization
Expand Down
5 changes: 0 additions & 5 deletions apps/webapp/app/v3/services/createOrgIntegration.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
import { OrganizationIntegration } from "@trigger.dev/database";
import { BaseService } from "./baseService.server";
import { WebClient } from "@slack/web-api";
import { env } from "~/env.server";
import { $transaction } from "~/db.server";
import { getSecretStore } from "~/services/secrets/secretStore.server";
import { generateFriendlyId } from "../friendlyIdentifiers";
import { OrgIntegrationRepository } from "~/models/orgIntegration.server";

export class CreateOrgIntegrationService extends BaseService {
Expand Down
Loading