Skip to content
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

feat: send test message #44

Merged
merged 3 commits into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions apps/api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
"@date-fns/tz": "^1.2.0",
"@date-fns/utc": "^2.1.0",
"@envelop/generic-auth": "^7.0.0",
"@envelop/rate-limiter": "^6.2.1",
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Missing required peer dependency for rate limiter

The @envelop/rate-limiter package requires @envelop/core ^5.0.2 as a peer dependency, but it's not found in package.json. Please add it to ensure proper functionality.

🔗 Analysis chain

Verify rate limiter package compatibility

The @envelop/rate-limiter package is appropriately added. Let's verify the version compatibility and check for any known issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for latest version and any security advisories
echo "Checking NPM for latest version..."
npm view @envelop/rate-limiter version

echo -e "\nChecking for known vulnerabilities..."
npm audit @envelop/rate-limiter

echo -e "\nChecking package compatibility with graphql-yoga..."
npm view [email protected] peerDependencies

Length of output: 799


Script:

#!/bin/bash
# Check peer dependencies and version requirements
echo "Checking @envelop/rate-limiter peer dependencies..."
npm view @envelop/rate-limiter peerDependencies

echo -e "\nChecking graphql version in package.json..."
cat apps/api/package.json | grep '"graphql":'

Length of output: 411


Script:

#!/bin/bash
# Check @envelop/core version
echo "Checking @envelop/core version in package.json..."
cat apps/api/package.json | grep '"@envelop/core":'

Length of output: 162

"@graphql-tools/load-files": "^7.0.0",
"@graphql-tools/merge": "^9.0.0",
"@logtail/pino": "^0.4.15",
Expand Down
13 changes: 13 additions & 0 deletions apps/api/src/app/digests/services/digest.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ import {
FindDigestByTypeArgs,
UpsertDigest,
Digest,
DigestWithRelations,
} from "./digest.types";
import { assign, isObject } from "radash";
import { DigestType } from "@prisma/client";
import { sendTeamMetricsDigest } from "./digest-team-metrics.service";
import { sendTeamWipDigest } from "./digest-team-wip.service";

export const findDigestByType = async <T extends DigestType>({
workspaceId,
Expand Down Expand Up @@ -100,3 +103,13 @@ export const upsertDigest = async ({

return updatedDigest as unknown as Digest;
};

export const sendDigest = async (digest: DigestWithRelations) => {
if (digest.type === DigestType.TEAM_METRICS) {
await sendTeamMetricsDigest(digest);
}

if (digest.type === DigestType.TEAM_WIP) {
await sendTeamWipDigest(digest);
}
};
Comment on lines +107 to +115
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and type safety.

The current implementation has several areas for improvement:

  1. No error handling for the service calls
  2. No handling for unknown digest types
  3. Type checking could be more maintainable
 export const sendDigest = async (digest: DigestWithRelations) => {
+  try {
-    if (digest.type === DigestType.TEAM_METRICS) {
-      await sendTeamMetricsDigest(digest);
-    }
-
-    if (digest.type === DigestType.TEAM_WIP) {
-      await sendTeamWipDigest(digest);
-    }
+    const digestHandlers = {
+      [DigestType.TEAM_METRICS]: sendTeamMetricsDigest,
+      [DigestType.TEAM_WIP]: sendTeamWipDigest,
+    } as const;
+
+    const handler = digestHandlers[digest.type];
+    if (!handler) {
+      throw new Error(`Unsupported digest type: ${digest.type}`);
+    }
+
+    await handler(digest);
+  } catch (error) {
+    throw new Error(`Failed to send digest: ${error.message}`);
+  }
 };
📝 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.

Suggested change
export const sendDigest = async (digest: DigestWithRelations) => {
if (digest.type === DigestType.TEAM_METRICS) {
await sendTeamMetricsDigest(digest);
}
if (digest.type === DigestType.TEAM_WIP) {
await sendTeamWipDigest(digest);
}
};
export const sendDigest = async (digest: DigestWithRelations) => {
try {
const digestHandlers = {
[DigestType.TEAM_METRICS]: sendTeamMetricsDigest,
[DigestType.TEAM_WIP]: sendTeamWipDigest,
} as const;
const handler = digestHandlers[digest.type];
if (!handler) {
throw new Error(`Unsupported digest type: ${digest.type}`);
}
await handler(digest);
} catch (error) {
throw new Error(`Failed to send digest: ${error.message}`);
}
};

12 changes: 2 additions & 10 deletions apps/api/src/app/digests/workers/digest-send.worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,8 @@ import { SweetQueue } from "../../../bull-mq/queues";
import { createWorker } from "../../../bull-mq/workers";
import { logger } from "../../../lib/logger";
import { DigestWithRelations } from "../services/digest.types";
import { DigestType } from "@prisma/client";
import { sendTeamWipDigest } from "../services/digest-team-wip.service";
import { sendTeamMetricsDigest } from "../services/digest-team-metrics.service";
import { InputValidationException } from "../../errors/exceptions/input-validation.exception";
import { sendDigest } from "../services/digest.service";

export const digestSendWorker = createWorker(
SweetQueue.DIGEST_SEND,
Expand All @@ -21,12 +19,6 @@ export const digestSendWorker = createWorker(
});
}

if (digest.type === DigestType.TEAM_METRICS) {
await sendTeamMetricsDigest(digest);
}

if (digest.type === DigestType.TEAM_WIP) {
await sendTeamWipDigest(digest);
}
await sendDigest(digest);
}
);
9 changes: 9 additions & 0 deletions apps/api/src/app/directives.schema.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export default /* GraphQL */ `
directive @rateLimit(
max: Int
window: String
message: String
identityArgs: [String]
arrayLengthField: String
) on FIELD_DEFINITION
`;
21 changes: 21 additions & 0 deletions apps/api/src/app/errors/exceptions/rate-limit.exception.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import {
BaseException,
BaseExceptionOptions,
ErrorCode,
} from "./base.exception";

export class RateLimitException extends BaseException {
name = "RateLimitException";

constructor(
message = "Rate limit exceeded",
options?: Partial<BaseExceptionOptions>
) {
super(message, {
code: ErrorCode.RATE_LIMIT_EXCEEDED,
userFacingMessage: "You have exceeded the rate limit.",
severity: "log",
...options,
});
}
}
12 changes: 12 additions & 0 deletions apps/api/src/app/integrations/resolvers/integrations.schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,24 @@ export default /* GraphQL */ `
app: IntegrationApp!
}

input SendTestMessageInput {
workspaceId: SweetID!
app: IntegrationApp!
channel: String!
}

extend type Workspace {
integrations: [Integration!]!
}

type Mutation {
installIntegration(input: InstallIntegrationInput!): Void
removeIntegration(input: RemoveIntegrationInput!): Void
sendTestMessage(input: SendTestMessageInput!): Void
@rateLimit(
window: "60s"
max: 5
message: "You got rate limited. You can send 5 test messages per minute."
)
}
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { z } from "zod";
import { createMutationResolver } from "../../../../lib/graphql";
import { logger } from "../../../../lib/logger";
import { validateInputOrThrow } from "../../../../lib/validate-input";
import { protectWithPaywall } from "../../../billing/services/billing.service";
import { authorizeWorkspaceOrThrow } from "../../../workspace-authorization.service";
import { sendTestMessage } from "../../slack/services/slack-integration.service";
import { IntegrationApp } from "@sweetr/graphql-types/api";

export const sendTestMessageMutation = createMutationResolver({
sendTestMessage: async (_, { input }, context) => {
logger.info("mutation.sendTestMessage", { input });

validateInputOrThrow(
z.object({
workspaceId: z.number(),
channel: z.string().max(80),
app: z.nativeEnum(IntegrationApp),
}),
Comment on lines +16 to +19
Copy link

@coderabbitai coderabbitai bot Jan 30, 2025

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove unused app parameter or implement multi-app support

The app parameter is validated but not used in the implementation. The service is currently Slack-specific with no support for other integration apps. Either:

  • Remove the app parameter if only Slack support is intended
  • Implement the functionality for other integration apps if multi-app support is planned
🔗 Analysis chain

Verify the usage of the app parameter.

The app parameter is validated but not passed to the sendTestMessage function. This could indicate:

  1. Missing functionality for other integration apps
  2. Unnecessary validation if only Slack is supported
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if sendTestMessage is implemented for other apps
rg -A 5 "sendTestMessage.*\(" apps/api/src/app/integrations/

Length of output: 2642

Copy link
Member Author

Choose a reason for hiding this comment

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

Just future-proofing for when we support MS Teams.

Copy link

Choose a reason for hiding this comment

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

@waltergalvao Ah, that makes perfect sense! Thanks for the clarification.

(`∀´)D


✏️ Learnings added
Learnt from: waltergalvao
PR: sweetr-dev/sweetr.dev#44
File: apps/api/src/app/integrations/resolvers/mutations/send-test-message.mutation.ts:16-19
Timestamp: 2025-01-30T06:09:54.631Z
Learning: The `app` parameter in the `sendTestMessage` mutation is included to support future MS Teams integration, even though it's currently only used with Slack.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: waltergalvao
PR: sweetr-dev/sweetr.dev#44
File: apps/api/src/app/integrations/resolvers/mutations/send-test-message.mutation.ts:30-30
Timestamp: 2025-01-30T06:09:36.043Z
Learning: GraphQL mutations in this codebase already handle errors and show user-facing error messages. Additional try-catch blocks in mutation resolvers are not needed as errors are properly propagated and handled at the GraphQL layer.

input
);

await authorizeWorkspaceOrThrow({
workspaceId: input.workspaceId,
gitProfileId: context.currentToken.gitProfileId,
});

await protectWithPaywall(input.workspaceId);

await sendTestMessage(input.workspaceId, input.channel);
Copy link

@coderabbitai coderabbitai bot Jan 30, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider error handling for sendTestMessage.

The service call should be wrapped in a try-catch block to handle potential integration errors gracefully.

-    await sendTestMessage(input.workspaceId, input.channel);
+    try {
+      await sendTestMessage(input.workspaceId, input.channel);
+    } catch (error) {
+      logger.error("Failed to send test message", { error, input });
+      throw error;
+    }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

GraphQL already handles this. We show a specific user facing error message

Copy link

Choose a reason for hiding this comment

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

@waltergalvao Understood, thanks for the clarification. The GraphQL layer's error handling is sufficient here.


✏️ Learnings added
Learnt from: waltergalvao
PR: sweetr-dev/sweetr.dev#44
File: apps/api/src/app/integrations/resolvers/mutations/send-test-message.mutation.ts:30-30
Timestamp: 2025-01-30T06:09:35.781Z
Learning: GraphQL mutations in this codebase already handle errors and show user-facing error messages. Additional try-catch blocks in mutation resolvers are not needed as errors are properly propagated and handled at the GraphQL layer.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


return null;
},
});
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ export const joinSlackChannel = async (
const channel = await findSlackChannel(slackClient, channelName);

if (!channel?.id) {
throw new ResourceNotFoundException("Slack channel not found");
throw new ResourceNotFoundException("Slack channel not found", {
userFacingMessage: "Could not find Slack channel.",
});
}

if (!channel.is_member) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,11 @@ import {
authorizeSlackWorkspace,
getSlackClient,
getWorkspaceSlackClient,
joinSlackChannel,
sendSlackMessage,
uninstallSlackWorkspace,
} from "./slack-client.service";
import { ResourceNotFoundException } from "../../../errors/exceptions/resource-not-found.exception";

export const installIntegration = async (workspaceId: number, code: string) => {
let response: OauthV2AccessResponse;
Expand Down Expand Up @@ -122,3 +125,20 @@ export const getInstallUrl = (): string => {

return url.toString();
};

export const sendTestMessage = async (workspaceId: number, channel: string) => {
const { slackClient } = await getWorkspaceSlackClient(workspaceId);

const slackChannel = await joinSlackChannel(slackClient, channel);

if (!slackChannel?.id) {
throw new ResourceNotFoundException("Slack channel not found");
}

await sendSlackMessage(slackClient, {
channel: slackChannel.id,
text: "Sweet, it works! 🍧",
unfurl_links: false,
unfurl_media: false,
});
};
12 changes: 12 additions & 0 deletions apps/api/src/lib/rate-limiter.plugin.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { useRateLimiter } from "@envelop/rate-limiter";
import { GraphQLContext } from "@sweetr/graphql-types/shared";
import { RateLimitException } from "../app/errors/exceptions/rate-limit.exception";

export const rateLimiterPlugin = useRateLimiter({
identifyFn: (context: GraphQLContext) =>
context.workspaceId?.toString() ?? context.currentToken?.userId.toString(),
transformError: (message: string) =>
new RateLimitException("Rate limit exceeded", {
userFacingMessage: message,
}),
});
3 changes: 2 additions & 1 deletion apps/api/src/yoga.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { join } from "path";
import { authPlugin } from "./app/auth/resolvers/plugins/auth.plugin";
import { env } from "./env";
import { useSentry } from "./lib/use-sentry.plugin";
import { rateLimiterPlugin } from "./lib/rate-limiter.plugin";

const resolvers = loadFilesSync(
join(__dirname, "./**/*.(query|mutation|resolver).(js|ts)")
Expand All @@ -21,7 +22,7 @@ const schema = createSchema<GraphQLContext>({
export const yoga = createYoga<GraphQLContext>({
graphqlEndpoint: "/",
schema,
plugins: [authPlugin, useSentry()],
plugins: [authPlugin, useSentry(), rateLimiterPlugin],
graphiql: env.NODE_ENV === "development",
landingPage: false,
logging: false,
Expand Down
23 changes: 23 additions & 0 deletions apps/web/src/api/integrations.api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import {
InstallIntegrationMutation,
MutationInstallIntegrationArgs,
MutationRemoveIntegrationArgs,
MutationSendTestMessageArgs,
RemoveIntegrationMutation,
SendTestMessageMutation,
WorkspaceIntegrationsQuery,
WorkspaceIntegrationsQueryVariables,
} from "@sweetr/graphql-types/frontend/graphql";
Expand Down Expand Up @@ -93,3 +95,24 @@ export const useRemoveIntegrationMutation = (
},
...options,
});

export const useSendTestMessageMutation = (
options?: UseMutationOptions<
SendTestMessageMutation,
unknown,
MutationSendTestMessageArgs,
unknown
>,
) =>
useMutation({
mutationFn: (args) =>
graphQLClient.request(
graphql(/* GraphQL */ `
mutation SendTestMessage($input: SendTestMessageInput!) {
sendTestMessage(input: $input)
}
`),
args,
),
...options,
});
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,32 @@ import {
Group,
Input,
Text,
Button,
Tooltip,
} from "@mantine/core";
import { DigestFrequency } from "@sweetr/graphql-types/api";
import { IconClock, IconInfoCircle, IconWorld } from "@tabler/icons-react";
import {
IconBrandSlack,
IconClock,
IconInfoCircle,
IconWorld,
} from "@tabler/icons-react";
import { BoxSetting } from "../../../../../../../components/box-setting";
import { SelectHour } from "../../../../../../../components/select-hour";
import { SelectTimezone } from "../../../../../../../components/select-timezone/select-timezone";
import { DayOfTheWeek } from "@sweetr/graphql-types/frontend/graphql";
import { useEffect, useRef } from "react";
import { useSendTestMessage } from "../../../../use-send-test-message";

interface DigestBaseFieldsProps {
form: UseFormReturnType<FormDigest>;
}

export const DigestBaseFields = ({ form }: DigestBaseFieldsProps) => {
const { sendTestMessage, isSendingTestMessage } = useSendTestMessage();
const isEnabled = form.values.enabled;

const channelRef = useRef<HTMLInputElement>(null);

useEffect(() => {
if (isEnabled && !form.values.channel) {
channelRef.current?.focus();
Expand Down Expand Up @@ -69,6 +77,18 @@ export const DigestBaseFields = ({ form }: DigestBaseFieldsProps) => {
</Input.Error>
)}
</Stack>
<Tooltip label="Send a test message to the channel" withArrow>
<Button
variant="default"
onClick={() => {
sendTestMessage(form.values.channel);
}}
leftSection={<IconBrandSlack size={16} stroke={1.5} />}
loading={isSendingTestMessage}
>
Test
</Button>
</Tooltip>
</Group>
</Input.Wrapper>
</>
Expand Down
39 changes: 39 additions & 0 deletions apps/web/src/app/teams/[id]/use-send-test-message.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { IntegrationApp } from "@sweetr/graphql-types/api";
import { showSuccessNotification } from "../../../providers/notification.provider";
import { getErrorMessage } from "../../../providers/error-message.provider";
import { useSendTestMessageMutation } from "../../../api/integrations.api";
import { useWorkspace } from "../../../providers/workspace.provider";
import { showErrorNotification } from "../../../providers/notification.provider";

export const useSendTestMessage = () => {
const { workspace } = useWorkspace();
const { mutate, isPending: isSendingTestMessage } =
useSendTestMessageMutation();

const sendTestMessage = (channel: string) => {
mutate(
{
input: {
workspaceId: workspace.id,
app: IntegrationApp.SLACK,
channel: channel,
},
},
{
onError: (error) => {
showErrorNotification({
message: getErrorMessage(error),
});
},
onSuccess: () => {
showSuccessNotification({ message: "Test message sent" });
},
},
);
};
Comment on lines +13 to +33
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation for the channel parameter.

The channel parameter should be validated before making the API call to provide immediate feedback to users.

   const sendTestMessage = (channel: string) => {
+    if (!channel?.trim()) {
+      showErrorNotification({
+        message: "Please enter a valid channel name",
+      });
+      return;
+    }
+
     mutate(
       {
         input: {
           workspaceId: workspace.id,
           app: IntegrationApp.SLACK,
-          channel: channel,
+          channel: channel.trim(),
         },
       },
📝 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.

Suggested change
const sendTestMessage = (channel: string) => {
mutate(
{
input: {
workspaceId: workspace.id,
app: IntegrationApp.SLACK,
channel: channel,
},
},
{
onError: (error) => {
showErrorNotification({
message: getErrorMessage(error),
});
},
onSuccess: () => {
showSuccessNotification({ message: "Test message sent" });
},
},
);
};
const sendTestMessage = (channel: string) => {
if (!channel?.trim()) {
showErrorNotification({
message: "Please enter a valid channel name",
});
return;
}
mutate(
{
input: {
workspaceId: workspace.id,
app: IntegrationApp.SLACK,
channel: channel.trim(),
},
},
{
onError: (error) => {
showErrorNotification({
message: getErrorMessage(error),
});
},
onSuccess: () => {
showSuccessNotification({ message: "Test message sent" });
},
},
);
};


return {
sendTestMessage,
isSendingTestMessage,
};
};
9 changes: 8 additions & 1 deletion apps/web/src/providers/error-message.provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,14 @@ export const getErrorMessage = (error: unknown) => {
}
| undefined;

if (extensions?.code === "INPUT_VALIDATION_FAILED") {
if (
extensions &&
[
"INPUT_VALIDATION_FAILED",
"RESOURCE_NOT_FOUND",
"RATE_LIMIT_EXCEEDED",
].includes(extensions.code)
) {
return extensions.userFacingMessage;
}
}
Expand Down
Loading