feat: send test message #44
Conversation
sweetr-dev
bot
added
the
large
WalkthroughThis pull request introduces a comprehensive implementation of test message sending functionality and rate limiting for the API. The changes span multiple files across the application, adding a new GraphQL mutation for sending test messages to Slack, implementing a rate limiter plugin, and creating supporting infrastructure like a new exception type and directive. The implementation includes both backend and frontend components to enable sending test messages and controlling API request rates. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 6
🧹 Nitpick comments (12)
apps/api/src/app/directives.schema.ts (1)
2-8: Consider improving type safety for the rate limit directive.The
windowparameter being a String type could lead to validation issues. Consider:
- Using an enum for predefined time windows, or
- Adding a custom scalar type for duration/interval
Also, consider adding GraphQL descriptions to document the purpose and usage of each parameter.
directive @rateLimit( + """Maximum number of requests allowed within the time window""" max: Int + """Time window for rate limiting (e.g., "60s", "1h")""" window: String + """Custom error message when rate limit is exceeded""" message: String + """Arguments used to identify the rate limit subject""" identityArgs: [String] + """Field name containing the array length for bulk operations""" arrayLengthField: String ) on FIELD_DEFINITIONapps/api/src/app/errors/exceptions/rate-limit.exception.ts (1)
14-19: Consider making severity level configurable.The severity level is hardcoded to "log", but rate limit violations might need different severity levels based on the context (e.g., potential abuse vs normal usage patterns).
super(message, { code: ErrorCode.RATE_LIMIT_EXCEEDED, userFacingMessage: "You have exceeded the rate limit.", - severity: "log", + severity: options?.severity ?? "log", ...options, });apps/api/src/lib/rate-limiter.plugin.ts (1)
8-11: Enhance error message handling.Consider adding validation for the error message and including rate limit details (remaining attempts, reset time) in the user-facing message.
transformError: (message: string) => new RateLimitException("Rate limit exceeded", { - userFacingMessage: message, + userFacingMessage: message.trim() || "You have exceeded the rate limit. Please try again later.", + extra: { originalMessage: message }, }),apps/api/src/app/digests/workers/digest-send.worker.ts (1)
22-22: Enhance error handling and add success logging.Consider adding try-catch block and success logging for better observability.
- await sendDigest(digest); + try { + await sendDigest(digest); + logger.info("digestSendWorker: Digest sent successfully", { + extra: { digestId: digest.id, type: digest.type }, + }); + } catch (error) { + logger.error("digestSendWorker: Failed to send digest", { + extra: { digestId: digest.id, type: digest.type, error }, + }); + throw error; + }apps/web/src/providers/error-message.provider.ts (1)
18-25: LGTM! Consider extracting error codes to a constant.The error handling implementation looks good and properly integrates with the new rate limiting feature. However, consider extracting the array of error codes to a named constant for better maintainability.
+const USER_FACING_ERROR_CODES = [ + "INPUT_VALIDATION_FAILED", + "RESOURCE_NOT_FOUND", + "RATE_LIMIT_EXCEEDED", +] as const; if ( extensions && - [ - "INPUT_VALIDATION_FAILED", - "RESOURCE_NOT_FOUND", - "RATE_LIMIT_EXCEEDED", - ].includes(extensions.code) + USER_FACING_ERROR_CODES.includes(extensions.code as typeof USER_FACING_ERROR_CODES[number]) ) {apps/api/src/app/integrations/resolvers/integrations.schema.ts (2)
26-30: LGTM! Consider adding field descriptions.The input type structure looks good, but consider adding descriptions for the fields to improve schema documentation.
input SendTestMessageInput { + "The ID of the workspace to send the test message to" workspaceId: SweetID! + "The integration app to use for sending the message" app: IntegrationApp! + "The channel where the test message will be sent" channel: String! }
39-44: Consider increasing the rate limit window.The current rate limit of 5 requests per minute might be too restrictive for users testing multiple channels. Consider increasing it to a more user-friendly limit, such as 10 requests per minute.
sendTestMessage(input: SendTestMessageInput!): Void @rateLimit( window: "60s" - max: 5 - message: "You got rate limited. You can send 5 test messages per minute." + max: 10 + message: "You got rate limited. You can send 10 test messages per minute." )apps/web/src/app/teams/[id]/use-send-test-message.ts (1)
17-19: Consider making the integration app configurable.The
appparameter is hardcoded toSLACK. If other integrations are planned, consider making this configurable.+ const sendTestMessage = (channel: string, app: IntegrationApp = IntegrationApp.SLACK) => { mutate( { input: { workspaceId: workspace.id, - app: IntegrationApp.SLACK, + app, channel: channel.trim(), }, },apps/api/src/app/integrations/slack/services/slack-client.service.ts (1)
73-75: Consider adding more context to the error message.The error message could be more helpful by including the channel name.
- throw new ResourceNotFoundException("Slack channel not found", { - userFacingMessage: "Could not find Slack channel.", + throw new ResourceNotFoundException(`Slack channel "${channelName}" not found`, { + userFacingMessage: `Could not find Slack channel "${channelName}".`, });apps/web/src/api/integrations.api.ts (1)
99-118: Consider adding query invalidation for consistency.Other mutation hooks in this file invalidate related queries in their
onSettledhandlers. Consider whether thesendTestMessagemutation should invalidate any queries to maintain UI consistency.Apply this diff to add query invalidation if needed:
export const useSendTestMessageMutation = ( options?: UseMutationOptions< SendTestMessageMutation, unknown, MutationSendTestMessageArgs, unknown >, ) => useMutation({ mutationFn: (args) => graphQLClient.request( graphql(/* GraphQL */ ` mutation SendTestMessage($input: SendTestMessageInput!) { sendTestMessage(input: $input) } `), args, ), + onSettled: () => { + queryClient.invalidateQueries({ + queryKey: ["integrations"], + }); + }, ...options, });apps/api/src/app/integrations/slack/services/slack-integration.service.ts (1)
129-144: Improve error handling and message configuration.The function implementation is good, but consider these improvements:
- Add error handling for
sendSlackMessage.- Externalize the test message text for easier maintenance and potential localization.
Apply this diff to improve the implementation:
+const TEST_MESSAGE = "Sweet, it works! 🍧"; + 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, - }); + try { + await sendSlackMessage(slackClient, { + channel: slackChannel.id, + text: TEST_MESSAGE, + unfurl_links: false, + unfurl_media: false, + }); + } catch (error) { + throw new IntegrationException("Failed to send test message", { + originalError: error, + }); + } };apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx (1)
80-91: Enhance button behavior and error handling.Consider these improvements to the test button implementation:
- Disable the button when the channel field is empty.
- Add error handling for the onClick handler.
Apply this diff to improve the implementation:
<Tooltip label="Send a test message to the channel" withArrow> <Button variant="default" + disabled={!form.values.channel} onClick={() => { + try { sendTestMessage(form.values.channel); + } catch (error) { + console.error('Failed to send test message:', error); + } }} leftSection={<IconBrandSlack size={16} stroke={1.5} />} loading={isSendingTestMessage} > Test </Button> </Tooltip>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/api/package.json(1 hunks)apps/api/src/app/digests/services/digest.service.ts(2 hunks)apps/api/src/app/digests/workers/digest-send.worker.ts(2 hunks)apps/api/src/app/directives.schema.ts(1 hunks)apps/api/src/app/errors/exceptions/rate-limit.exception.ts(1 hunks)apps/api/src/app/integrations/resolvers/integrations.schema.ts(1 hunks)apps/api/src/app/integrations/resolvers/mutations/send-test-message.mutation.ts(1 hunks)apps/api/src/app/integrations/slack/services/slack-client.service.ts(1 hunks)apps/api/src/app/integrations/slack/services/slack-integration.service.ts(2 hunks)apps/api/src/lib/rate-limiter.plugin.ts(1 hunks)apps/api/src/yoga.ts(2 hunks)apps/web/src/api/integrations.api.ts(2 hunks)apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx(2 hunks)apps/web/src/app/teams/[id]/use-send-test-message.ts(1 hunks)apps/web/src/providers/error-message.provider.ts(1 hunks)packages/graphql-types/api.ts(8 hunks)packages/graphql-types/frontend/gql.ts(2 hunks)packages/graphql-types/frontend/graphql.ts(5 hunks)packages/graphql-types/shared.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
packages/graphql-types/shared.ts (1)
37-37: LGTM! Error code addition looks good.The new
RATE_LIMIT_EXCEEDEDerror code is properly added to the enum, maintaining consistent style and alphabetical ordering.apps/api/src/yoga.ts (1)
9-9: LGTM! Plugin integration looks good.The rate limiter plugin is properly imported and integrated with the existing plugins array.
Also applies to: 25-25
apps/api/src/app/integrations/slack/services/slack-client.service.ts (1)
Line range hint
47-54: Address the pagination TODO comment.The current implementation has a hard limit of 1000 channels which might not be sufficient for large workspaces.
Would you like me to implement pagination for the channel list? This would involve:
- Using cursor-based pagination with Slack's API
- Implementing a channel search function
- Adding proper error handling for rate limits
apps/web/src/api/integrations.api.ts (1)
13-13: LGTM!The new type imports are correctly added and properly organized with the existing imports.
Also applies to: 15-15
apps/api/src/app/integrations/slack/services/slack-integration.service.ts (1)
14-15: LGTM!The imports are correctly organized and the
ResourceNotFoundExceptionis appropriately used for the channel not found case.Also applies to: 18-18
apps/web/src/app/teams/[id]/digests/settings/components/digest-base-fields/digest-base-fields.tsx (2)
15-16: LGTM!The imports are well-organized and the Mantine UI components are appropriately imported.
Also applies to: 19-24, 30-30
37-37: LGTM!The hook is correctly destructured to access both the action and loading state.
packages/graphql-types/frontend/gql.ts (2)
36-36: LGTM!The SendTestMessage mutation document is correctly added to the documents map.
150-153: LGTM!The type export for the SendTestMessage mutation is properly defined and follows the existing pattern.
packages/graphql-types/api.ts (4)
250-250: LGTM: Appropriate return type for the mutationThe
Maybe<Void>return type is well-chosen for this action-based mutation, allowing for both success (null) and error (undefined) cases.
524-528: Consider adding channel format validationThe input type structure is good, but consider validating the channel format on the resolver level since different apps may have different channel format requirements (e.g., Slack channels starting with '#').
968-974: LGTM: Well-structured rate limiting configurationThe rate limit directive arguments provide a comprehensive and flexible configuration structure with all necessary parameters for effective rate limiting.
976-976: LGTM: Properly typed directive resolverThe resolver type is correctly defined with appropriate context and argument types.
packages/graphql-types/frontend/graphql.ts (3)
887-889: LGTM: Properly typed mutation variablesThe mutation variables are correctly typed using the SendTestMessageInput type.
892-892: LGTM: Consistent return typeThe mutation return type correctly matches the API schema definition.
1040-1040: LGTM: Well-defined GraphQL documentThe document is properly typed and structured for use with GraphQL clients.
| identifyFn: (context: GraphQLContext) => | ||
| context.workspaceId?.toString() ?? context.currentToken.userId.toString(), |
There was a problem hiding this comment.
Add null checks to prevent runtime errors.
The identifyFn lacks null checks for context.currentToken, which could cause runtime errors if the token is not present.
identifyFn: (context: GraphQLContext) =>
- context.workspaceId?.toString() ?? context.currentToken.userId.toString(),
+ context.workspaceId?.toString() ?? context.currentToken?.userId?.toString() ?? 'anonymous',📝 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.
| identifyFn: (context: GraphQLContext) => | |
| context.workspaceId?.toString() ?? context.currentToken.userId.toString(), | |
| identifyFn: (context: GraphQLContext) => | |
| context.workspaceId?.toString() ?? context.currentToken?.userId?.toString() ?? 'anonymous', |
|
|
||
| await protectWithPaywall(input.workspaceId); | ||
|
|
||
| await sendTestMessage(input.workspaceId, input.channel); |
There was a problem hiding this comment.
🛠️ 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.
There was a problem hiding this comment.
GraphQL already handles this. We show a specific user facing error message
There was a problem hiding this comment.
@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.
| workspaceId: z.number(), | ||
| channel: z.string().max(80), | ||
| app: z.nativeEnum(IntegrationApp), | ||
| }), |
There was a problem hiding this comment.
💡 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
appparameter 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:
- Missing functionality for other integration apps
- 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
There was a problem hiding this comment.
Just future-proofing for when we support MS Teams.
There was a problem hiding this comment.
@waltergalvao Ah, that makes perfect sense! Thanks for the clarification.
(｀∀´)Ｄ
✏️ 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.
| 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" }); | ||
| }, | ||
| }, | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ 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.
| 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" }); | |
| }, | |
| }, | |
| ); | |
| }; |
| export const sendDigest = async (digest: DigestWithRelations) => { | ||
| if (digest.type === DigestType.TEAM_METRICS) { | ||
| await sendTeamMetricsDigest(digest); | ||
| } | ||
|
|
||
| if (digest.type === DigestType.TEAM_WIP) { | ||
| await sendTeamWipDigest(digest); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety.
The current implementation has several areas for improvement:
- No error handling for the service calls
- No handling for unknown digest types
- 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.
| 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}`); | |
| } | |
| }; |
| @@ -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", | |||
There was a problem hiding this comment.
💡 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
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates