feat: slack notifications #66
Conversation
📝 WalkthroughWalkthroughThe changes add a notifier system across the project. New environment variables have been introduced to configure the notifier provider (either "slack" or "null"), the Slack webhook URL, and a log level. Environment schema validations have been updated to require a webhook URL when the notifier is set to Slack. Service modules have been modified to pass and use a notifier dependency, allowing components like the Orchestrator to send error notifications. Within the shared package, several notifier interfaces and classes have been added, including implementations for Slack and a no-operation null notifier, as well as a factory class to instantiate the appropriate notifier based on configuration. Documentation and dependency files have been updated accordingly, and test suites have been added to cover the new notifier functionality. ✨ Finishing Touches
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 (13)
packages/shared/test/notifier/null.notifier.spec.ts (1)
7-22: Add more test cases for comprehensive coverage.While the current test is good, consider adding more test cases to verify behavior with:
- Missing optional context properties
- Different chainId values
- Different message content
Example additional test:
it("handles missing optional context properties", async () => { const notifier = new NullNotifier(); const context: NotifierContext = { chainId: 1 as ChainId, }; const consoleSpy = vi.spyOn(console, "log"); const result = await notifier.send("test message", context); expect(result).toBeUndefined(); expect(consoleSpy).not.toHaveBeenCalled(); });packages/shared/src/notifier/notifier.factory.ts (1)
8-29: Consider converting static-only class to a function.The
NotifierFactoryclass contains only static members. Consider refactoring it to a simple function for better maintainability and to align with functional programming principles.-export class NotifierFactory { - static create(options: NotifierConfig<NotifierProvider>, logger: ILogger): INotifier { +export function createNotifier(options: NotifierConfig<NotifierProvider>, logger: ILogger): INotifier { let notifier: INotifier; switch (options.notifierProvider) { case "slack": notifier = new SlackNotifier(options.opts, logger); break; // Can add more notification services here, like TelegramNotifier, etc. default: notifier = new NullNotifier(); } return notifier; - } -}🧰 Tools
🪛 Biome (1.9.4)
[error] 8-29: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/shared/test/notifier/notifier.factory.spec.ts (1)
17-27: Add test for invalid webhook URL.Consider adding a test case to verify the behavior when an invalid webhook URL is provided.
it("validates webhook URL format for Slack notifier", () => { const config: NotifierConfig<"slack"> = { notifierProvider: "slack", opts: { webhookUrl: "invalid-url", }, }; expect(() => NotifierFactory.create(config, logger)).toThrow(); });apps/processing/src/config/env.ts (1)
43-45: Consider stricter URL format validation for Slack webhook.The current URL validation is basic. Consider adding a pattern check to ensure it's a valid Slack webhook URL.
- SLACK_WEBHOOK_URL: z.string().url().optional(), + SLACK_WEBHOOK_URL: z.string() + .url() + .regex(/^https:\/\/hooks\.slack\.com\/services\/.*$/, 'Must be a valid Slack webhook URL') + .optional(),packages/shared/src/notifier/slack.notifier.ts (1)
12-41: Consider enhancing error message formatting.The default formatter could be improved to provide more context and better readability in Slack messages.
Consider this enhanced formatting:
- text: `An error has occurred on Chain ${info.context.chainId} on event ${info.context.event?.eventName || "unknown"}`, + text: `🚨 *Error Alert*\n• Chain: ${info.context.chainId}\n• Event: ${info.context.event?.eventName || "unknown"}\n• Timestamp: ${new Date().toISOString()}`,packages/shared/test/notifier/slack.notifier.spec.ts (2)
39-45: Enhance minimal options test coverage.The test for minimal options initialization could be more thorough.
Consider adding assertions for default values:
it("initializes with default values when minimal options provided", () => { const minimalNotifier = new SlackNotifier( { webhookUrl: "https://hooks.slack.com/test" }, logger, ); expect(minimalNotifier).toBeDefined(); + expect(minimalNotifier["username"]).toBe("Alert BOT"); + expect(minimalNotifier["iconEmoji"]).toBe(":rotating_light:"); + expect(minimalNotifier["iconUrl"]).toBeUndefined(); + expect(minimalNotifier["formatter"]).toBeDefined(); });
96-110: Add test cases for transient errors.The error handling test could be expanded to cover different error scenarios.
Add test cases for transient errors:
it("handles transient errors appropriately", async () => { const timeoutError = new Error("Request timeout"); vi.mocked(mockWebhook.send).mockRejectedValueOnce(timeoutError); const message = "Test error message"; const context: NotifierContext = { chainId: 1 as ChainId, event: { eventName: "ProfileCreated" } as AnyIndexerFetchedEvent, }; await expect(notifier.send(message, context)).rejects.toThrow(); expect(logger.warn).toHaveBeenCalled(); });packages/data-flow/src/orchestrator.ts (1)
234-240: Consider adding stack trace for unknown errors.While the current implementation captures the error message and event details, adding a stack trace (if available) could help with debugging unknown errors.
void this.notifier.send( `Error processing event: ${stringify(event)} ${error}`, { chainId: this.chainId, event: event!, + stack: error instanceof Error ? error.stack : undefined, }, );packages/data-flow/test/unit/orchestrator.spec.ts (1)
80-82: Add tests for notification functionality.While the notifier mock is properly set up, consider adding explicit tests to verify that notifications are sent with the correct parameters in error scenarios.
Example test case:
it('sends notification for retriable errors', async () => { const retriableError = new RateLimitError({ className: 'ExternalProvider' }, 10); const mockEvent = createMockEvent('Allo', 'PoolCreated' as AlloEvent, 1); vi.spyOn(orchestrator['eventsProcessor'], 'processEvent') .mockRejectedValueOnce(retriableError); runPromise = orchestrator.run(abortController.signal); await vi.waitFor(() => { if (!notifier.send.mock.calls.length) throw new Error('Not yet called'); }); expect(notifier.send).toHaveBeenCalledWith( expect.any(String), expect.objectContaining({ chainId, event: mockEvent, stack: expect.any(String), }), ); });apps/processing/.env.example (1)
23-24: Enhance environment variable documentation.Consider adding more descriptive comments to clarify:
- The purpose of each notifier provider option
- The format expected for the Slack webhook URL
-NOTIFIER_PROVIDER=# 'slack' or 'null' -SLACK_WEBHOOK_URL= +# Notification provider: 'slack' for Slack notifications, 'null' for no notifications +NOTIFIER_PROVIDER= +# Slack Incoming Webhook URL (required when NOTIFIER_PROVIDER='slack') +# Format: https://hooks.slack.com/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX +SLACK_WEBHOOK_URL=packages/shared/README.md (3)
42-44: Section Header ClarityThe new "Configuring the Notifier" section heading is clear. Consider adding a brief introductory sentence that explains the purpose of this section (i.e., an overview of notifier configuration options) to provide additional context for the reader.
45-48: Slack Notifier Instructions – Wording RefinementThe Slack subsection is informative and provides a useful link to the Slack API docs. However, the phrase "get a webhook URL" is mentioned twice. Consider consolidating the wording for clarity. For example:
-Visit [Slack API docs](https://api.slack.com/messaging/webhooks) to create a Slack app and get a webhook URL. Follow the instructions for setting up a Slack app and get a webhook URL. Also, you can further customize the bot name, icons and scopes. +Visit [Slack API docs](https://api.slack.com/messaging/webhooks) to create a Slack app and obtain your webhook URL. You can then customize the bot name, icons, and scopes as needed.Additionally, you might mention the related environment variables (e.g.,
NOTIFIER_PROVIDERandSLACK_WEBHOOK_URL) to tie the documentation directly to the configuration setup.
49-51: Null Notifier Description EnhancementThe description for the "Null" notifier is concise and effectively communicates that it is a no-op. For added clarity, consider including a brief note on its intended usage (for example, in development or testing scenarios where notifications should be disabled).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
apps/processing/.env.example(1 hunks)apps/processing/src/config/env.ts(2 hunks)apps/processing/src/services/processing.service.ts(2 hunks)apps/processing/src/services/sharedDependencies.service.ts(4 hunks)packages/data-flow/src/orchestrator.ts(5 hunks)packages/data-flow/test/unit/orchestrator.spec.ts(3 hunks)packages/shared/README.md(1 hunks)packages/shared/package.json(1 hunks)packages/shared/src/external.ts(1 hunks)packages/shared/src/internal.ts(1 hunks)packages/shared/src/notifier/index.ts(1 hunks)packages/shared/src/notifier/notifier.factory.ts(1 hunks)packages/shared/src/notifier/notifier.interface.ts(1 hunks)packages/shared/src/notifier/notifierConfig.interface.ts(1 hunks)packages/shared/src/notifier/null.notifier.ts(1 hunks)packages/shared/src/notifier/slack.notifier.ts(1 hunks)packages/shared/test/notifier/notifier.factory.spec.ts(1 hunks)packages/shared/test/notifier/null.notifier.spec.ts(1 hunks)packages/shared/test/notifier/slack.notifier.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/shared/src/notifier/index.ts
- packages/shared/src/notifier/null.notifier.ts
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.ts`: Review TypeScript files for adherence to the fo...
**/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoidany; useunknownwhen necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have anexternal.tsfile explicitly listing public exports.
- Usebigintas-is; cast toNumberonly when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/shared/src/internal.tspackages/shared/src/external.tspackages/shared/src/notifier/notifierConfig.interface.tspackages/shared/src/notifier/notifier.interface.tspackages/data-flow/src/orchestrator.tsapps/processing/src/services/processing.service.tspackages/shared/src/notifier/notifier.factory.tspackages/shared/test/notifier/notifier.factory.spec.tsapps/processing/src/config/env.tspackages/data-flow/test/unit/orchestrator.spec.tspackages/shared/test/notifier/null.notifier.spec.tsapps/processing/src/services/sharedDependencies.service.tspackages/shared/src/notifier/slack.notifier.tspackages/shared/test/notifier/slack.notifier.spec.ts
`**/services/**/*.ts`: Review service files with the followi...
**/services/**/*.ts: Review service files with the following considerations:
- A Service encapsulates a broader business workflow and might orchestrate various components or interact with multiple data sources or APIs.
- Ensure proper composability: Services should use Providers for data/resource access and apply business/orchestration logic.
- Validate naming conventions for aggregating services (e.g.,AggregatorServiceor domain-specific names likeMetricsService).
apps/processing/src/services/processing.service.tsapps/processing/src/services/sharedDependencies.service.ts
`**/*.spec.ts`: Review the unit test files with the followin...
**/*.spec.ts: Review the unit test files with the following guidelines:
- Avoid using the word "should" in test descriptions.
- Ensure descriptive test names convey the intent of each test.
- Validate adherence to the Mocha/Chai/Jest test library best practices.
- Be concise and avoid overly nitpicky feedback outside of these best practices.
packages/shared/test/notifier/notifier.factory.spec.tspackages/data-flow/test/unit/orchestrator.spec.tspackages/shared/test/notifier/null.notifier.spec.tspackages/shared/test/notifier/slack.notifier.spec.ts
🪛 Biome (1.9.4)
packages/shared/src/notifier/notifier.factory.ts
[error] 8-29: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
apps/processing/src/services/sharedDependencies.service.ts
[error] 61-61: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (11)
packages/shared/src/notifier/notifierConfig.interface.ts (1)
3-3: LGTM!The type correctly defines the supported notifier providers and aligns with the PR objectives.
packages/shared/src/internal.ts (1)
12-12: LGTM!The export statement follows the established pattern and correctly exposes the notifier module.
packages/shared/src/notifier/notifier.factory.ts (1)
15-28: LGTM! Clean and extensible implementation.The factory implementation is well-structured and follows SOLID principles:
- Single Responsibility: Creates notifier instances
- Open/Closed: Easy to extend for new notification providers
- Dependency Injection: Logger is passed as a dependency
packages/shared/test/notifier/notifier.factory.spec.ts (1)
9-36: LGTM! Well-structured test suite.The test suite follows best practices:
- Clear test descriptions without "should"
- Proper mocking of dependencies
- Good coverage of main use cases
packages/shared/src/external.ts (1)
33-39: LGTM! Well-organized exports.The notifier exports are properly organized and follow the internal module pattern.
apps/processing/src/config/env.ts (1)
73-81: LGTM! Robust environment validation.The validation logic effectively ensures that the Slack webhook URL is provided when needed, using Zod's runtime type checking.
apps/processing/src/services/processing.service.ts (1)
88-89: Verify notifier integration with orchestrator.The notifier integration looks correct. Ensure the orchestrator properly handles the notifier in error scenarios.
Run this script to verify error handling:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check orchestrator's error handling with notifier # Search for error handling in orchestrator ast-grep --pattern 'catch (error) { $$$ notifier.send($$$) $$$ }'Length of output: 73
Manually verify error handling in orchestrator for notifier integration.
The automated search did not reveal explicit error catch blocks invoking notifier methods (e.g., via a call like
notifier.send(...)). Although the notifier is correctly passed into the orchestrator (as seen in the change at lines 88–89), please manually review the orchestrator’s error handling flow to confirm that it properly employs the notifier in error scenarios.packages/data-flow/src/orchestrator.ts (3)
204-208: LGTM! Comprehensive error notification for retriable errors.The notification includes all necessary context: error message, chain ID, event details, and full stack trace.
220-224: LGTM! Proper error notification for standard errors.The notification includes the essential context: error message, chain ID, event details, and stack trace.
105-105: LGTM! Clean dependency injection of the notifier.The notifier is properly injected as a dependency, following the existing pattern and SOLID principles.
packages/shared/package.json (1)
31-31: LGTM! Proper dependency management.The @slack/webhook dependency is correctly added with a pinned version for reproducibility.
| export type SlackNotifierConfig = { | ||
| notifierProvider: "slack"; | ||
| opts: SlackOptions; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc documentation for the type.
As per the coding guidelines, please add JSDoc documentation to describe the purpose and properties of the SlackNotifierConfig type.
Apply this diff to add documentation:
+/**
+ * Configuration type for the Slack notifier.
+ * @property notifierProvider - Must be set to "slack" to use Slack notifications.
+ * @property opts - Slack-specific configuration options.
+ */
export type SlackNotifierConfig = {
notifierProvider: "slack";
opts: SlackOptions;
};📝 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 type SlackNotifierConfig = { | |
| notifierProvider: "slack"; | |
| opts: SlackOptions; | |
| }; | |
| /** | |
| * Configuration type for the Slack notifier. | |
| * @property notifierProvider - Must be set to "slack" to use Slack notifications. | |
| * @property opts - Slack-specific configuration options. | |
| */ | |
| export type SlackNotifierConfig = { | |
| notifierProvider: "slack"; | |
| opts: SlackOptions; | |
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
@0xnigir1 Perfect! The JSDoc documentation has been added correctly. This will help improve code maintainability and developer experience.
🏁 Script executed:
#!/bin/bash
# Fetch the commit details
gh api /repos/defi-wonderland/grants-stack-indexer-v2/commits/e839723015f662867708394b3c09e21e385040c1 | jq -r '.files[] | select(.filename == "packages/shared/src/notifier/notifierConfig.interface.ts") | .patch'
Length of output: 1009
@0xnigir1 Excellent work! Not only did you add the suggested documentation for SlackNotifierConfig, but you also took the initiative to document the NotifierConfig generic type. The documentation is clear and comprehensive, especially the explanation of the generic type parameter. This will greatly help other developers understand the configuration system. 👏
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| export type NotifierConfig<Source extends NotifierProvider> = Source extends "slack" | ||
| ? SlackNotifierConfig | ||
| : { | ||
| notifierProvider: "null"; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc documentation for the generic type.
As per the coding guidelines, please add JSDoc documentation to describe the purpose and type parameter of the NotifierConfig type.
Apply this diff to add documentation:
+/**
+ * Generic configuration type for notifiers.
+ * @template Source - The type of notifier provider to configure.
+ * @returns Configuration type specific to the provider: SlackNotifierConfig for "slack", or a null configuration otherwise.
+ */
export type NotifierConfig<Source extends NotifierProvider> = Source extends "slack"
? SlackNotifierConfig
: {
notifierProvider: "null";
};📝 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 type NotifierConfig<Source extends NotifierProvider> = Source extends "slack" | |
| ? SlackNotifierConfig | |
| : { | |
| notifierProvider: "null"; | |
| }; | |
| /** | |
| * Generic configuration type for notifiers. | |
| * @template Source - The type of notifier provider to configure. | |
| * @returns Configuration type specific to the provider: SlackNotifierConfig for "slack", or a null configuration otherwise. | |
| */ | |
| export type NotifierConfig<Source extends NotifierProvider> = Source extends "slack" | |
| ? SlackNotifierConfig | |
| : { | |
| notifierProvider: "null"; | |
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
@0xnigir1 Thank you for implementing the suggested changes! The JSDoc documentation has been added correctly.
🏁 Script executed:
#!/bin/bash
# Fetch the specific commit changes
gh pr view 66 --json commits | jq -r '.commits[] | select(.oid == "e839723015f662867708394b3c09e21e385040c1") | .messageHeadline'
# Check the content of the file after the commit
git show e839723015f662867708394b3c09e21e385040c1:packages/shared/src/notifier/notifierConfig.interface.ts
Length of output: 1058
@0xnigir1 Thank you for implementing the suggested changes and going the extra mile! Not only did you add the requested JSDoc for NotifierConfig, but you also added helpful documentation for SlackNotifierConfig. This comprehensive approach to documentation is excellent.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| export type NotifierContext = { | ||
| event?: AnyIndexerFetchedEvent; | ||
| chainId: ChainId; | ||
| stack?: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc documentation for the type.
As per the coding guidelines, please add JSDoc documentation to describe the purpose and properties of the NotifierContext type.
Apply this diff to add documentation:
+/**
+ * Context information for notifications.
+ * @property event - Optional event that triggered the notification.
+ * @property chainId - The blockchain network ID where the event occurred.
+ * @property stack - Optional stack trace for error notifications.
+ */
export type NotifierContext = {
event?: AnyIndexerFetchedEvent;
chainId: ChainId;
stack?: string;
};📝 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 type NotifierContext = { | |
| event?: AnyIndexerFetchedEvent; | |
| chainId: ChainId; | |
| stack?: string; | |
| }; | |
| /** | |
| * Context information for notifications. | |
| * @property event - Optional event that triggered the notification. | |
| * @property chainId - The blockchain network ID where the event occurred. | |
| * @property stack - Optional stack trace for error notifications. | |
| */ | |
| export type NotifierContext = { | |
| event?: AnyIndexerFetchedEvent; | |
| chainId: ChainId; | |
| stack?: string; | |
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
@0xnigir1 Thank you for implementing the suggested changes! The JSDoc documentation has been added correctly.
🏁 Script executed:
#!/bin/bash
# Fetch the specific commit changes
gh pr view 66 --json commits --jq '.commits[] | select(.oid == "e839723015f662867708394b3c09e21e385040c1") | .messageHeadline'
git show e839723015f662867708394b3c09e21e385040c1 -- packages/shared/src/notifier/notifier.interface.ts
Length of output: 1499
@0xnigir1 Thank you for implementing the documentation changes! I see you've gone above and beyond by also improving the documentation for the INotifier interface's send method. The changes look great!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| export interface INotifier { | ||
| /** | ||
| * Sends a notification to the notifier provider. | ||
| * @param message - The message to notify. | ||
| */ | ||
| send(message: string, context: NotifierContext): Promise<void>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance JSDoc documentation for the interface.
The JSDoc documentation should include descriptions for all parameters and the return type.
Apply this diff to enhance documentation:
export interface INotifier {
/**
* Sends a notification to the notifier provider.
* @param message - The message to notify.
+ * @param context - The context information for the notification.
+ * @returns A promise that resolves when the notification is sent.
*/
send(message: string, context: NotifierContext): Promise<void>;
}📝 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 interface INotifier { | |
| /** | |
| * Sends a notification to the notifier provider. | |
| * @param message - The message to notify. | |
| */ | |
| send(message: string, context: NotifierContext): Promise<void>; | |
| } | |
| export interface INotifier { | |
| /** | |
| * Sends a notification to the notifier provider. | |
| * @param message - The message to notify. | |
| * @param context - The context information for the notification. | |
| * @returns A promise that resolves when the notification is sent. | |
| */ | |
| send(message: string, context: NotifierContext): Promise<void>; | |
| } |
There was a problem hiding this comment.
🧩 Analysis chain
@0xnigir1 Thank you for implementing the JSDoc improvements! The documentation is now more comprehensive and clearer.
(｀∇´ノ)ノ 🐰✨
🏁 Script executed:
#!/bin/bash
# Check the specific commit for changes
git show e839723015f662867708394b3c09e21e385040c1
Length of output: 2379
@0xnigir1 Excellent work! Not only did you implement the suggested JSDoc improvements for the INotifier interface, but you also took the initiative to add comprehensive documentation for NotifierContext, SlackNotifierConfig, and NotifierConfig types. This makes the codebase much more maintainable and developer-friendly.
(◕ᴗ◕✿) 🐰
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| /** @inheritdoc */ | ||
| async send(message: string, context: NotifierContext): Promise<void> { | ||
| const payload: IncomingWebhookSendArguments = { | ||
| username: this.username, | ||
| icon_emoji: this.iconEmoji, | ||
| icon_url: this.iconUrl, | ||
| }; | ||
|
|
||
| const formattedPayload = this.formatter({ message, context }); | ||
| Object.assign(payload, formattedPayload); | ||
|
|
||
| try { | ||
| await this.webhook.send(payload); | ||
| this.logger.debug("Notification sent to Slack", { | ||
| className: SlackNotifier.name, | ||
| }); | ||
| } catch (error) { | ||
| const errorMessage = isNativeError(error) ? error.message : stringify(error); | ||
| this.logger.error(`Failed to send notification: ${errorMessage}`, { | ||
| className: SlackNotifier.name, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in send method.
The error handling could be improved to provide more context and potentially retry on transient errors.
Consider this enhanced implementation:
async send(message: string, context: NotifierContext): Promise<void> {
const payload: IncomingWebhookSendArguments = {
username: this.username,
icon_emoji: this.iconEmoji,
icon_url: this.iconUrl,
};
const formattedPayload = this.formatter({ message, context });
Object.assign(payload, formattedPayload);
try {
await this.webhook.send(payload);
this.logger.debug("Notification sent to Slack", {
className: SlackNotifier.name,
+ context,
});
} catch (error) {
const errorMessage = isNativeError(error) ? error.message : stringify(error);
+ // Check for specific error types that might warrant a retry
+ const isTransientError = error instanceof Error &&
+ (error.message.includes('timeout') || error.message.includes('rate limit'));
+ const logLevel = isTransientError ? 'warn' : 'error';
+
- this.logger.error(`Failed to send notification: ${errorMessage}`, {
+ this.logger[logLevel](`Failed to send notification: ${errorMessage}`, {
className: SlackNotifier.name,
+ context,
+ isTransientError,
});
+
+ if (isTransientError) {
+ // Could integrate with retry strategy here
+ throw error;
+ }
}
}📝 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.
| /** @inheritdoc */ | |
| async send(message: string, context: NotifierContext): Promise<void> { | |
| const payload: IncomingWebhookSendArguments = { | |
| username: this.username, | |
| icon_emoji: this.iconEmoji, | |
| icon_url: this.iconUrl, | |
| }; | |
| const formattedPayload = this.formatter({ message, context }); | |
| Object.assign(payload, formattedPayload); | |
| try { | |
| await this.webhook.send(payload); | |
| this.logger.debug("Notification sent to Slack", { | |
| className: SlackNotifier.name, | |
| }); | |
| } catch (error) { | |
| const errorMessage = isNativeError(error) ? error.message : stringify(error); | |
| this.logger.error(`Failed to send notification: ${errorMessage}`, { | |
| className: SlackNotifier.name, | |
| }); | |
| } | |
| } | |
| /** @inheritdoc */ | |
| async send(message: string, context: NotifierContext): Promise<void> { | |
| const payload: IncomingWebhookSendArguments = { | |
| username: this.username, | |
| icon_emoji: this.iconEmoji, | |
| icon_url: this.iconUrl, | |
| }; | |
| const formattedPayload = this.formatter({ message, context }); | |
| Object.assign(payload, formattedPayload); | |
| try { | |
| await this.webhook.send(payload); | |
| this.logger.debug("Notification sent to Slack", { | |
| className: SlackNotifier.name, | |
| context, | |
| }); | |
| } catch (error) { | |
| const errorMessage = isNativeError(error) ? error.message : stringify(error); | |
| // Check for specific error types that might warrant a retry | |
| const isTransientError = error instanceof Error && | |
| (error.message.includes('timeout') || error.message.includes('rate limit')); | |
| const logLevel = isTransientError ? 'warn' : 'error'; | |
| this.logger[logLevel](`Failed to send notification: ${errorMessage}`, { | |
| className: SlackNotifier.name, | |
| context, | |
| isTransientError, | |
| }); | |
| if (isTransientError) { | |
| // Could integrate with retry strategy here | |
| throw error; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
this is a nth, let's keep it simple for now
There was a problem hiding this comment.
@0xnigir1 Understood, we'll keep the current simple implementation. Thanks for the feedback!
✏️ Learnings added
Learnt from: 0xnigir1
PR: defi-wonderland/grants-stack-indexer-v2#66
File: packages/shared/src/notifier/slack.notifier.ts:77-99
Timestamp: 2025-02-11T20:33:14.198Z
Learning: The Slack notifier implementation should be kept simple as it's a non-critical (nth) feature, with basic error handling being sufficient for now.
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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| static getNotifierOptions(env: Environment): NotifierConfig<NotifierProvider> { | ||
| if (env.NOTIFIER_PROVIDER === "slack") { | ||
| return { | ||
| notifierProvider: env.NOTIFIER_PROVIDER, | ||
| opts: { | ||
| webhookUrl: env.SLACK_WEBHOOK_URL!, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| notifierProvider: "null", | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix static method reference and enhance error handling.
The static method has issues with this reference and could benefit from better error handling.
Apply these improvements:
- static getNotifierOptions(env: Environment): NotifierConfig<NotifierProvider> {
+ private static getNotifierOptions(env: Environment): NotifierConfig<NotifierProvider> {
if (env.NOTIFIER_PROVIDER === "slack") {
+ if (!env.SLACK_WEBHOOK_URL) {
+ throw new Error("SLACK_WEBHOOK_URL is required when NOTIFIER_PROVIDER is 'slack'");
+ }
return {
notifierProvider: env.NOTIFIER_PROVIDER,
opts: {
- webhookUrl: env.SLACK_WEBHOOK_URL!,
+ webhookUrl: env.SLACK_WEBHOOK_URL,
},
};
}
return {
notifierProvider: "null",
};
}📝 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.
| static getNotifierOptions(env: Environment): NotifierConfig<NotifierProvider> { | |
| if (env.NOTIFIER_PROVIDER === "slack") { | |
| return { | |
| notifierProvider: env.NOTIFIER_PROVIDER, | |
| opts: { | |
| webhookUrl: env.SLACK_WEBHOOK_URL!, | |
| }, | |
| }; | |
| } | |
| return { | |
| notifierProvider: "null", | |
| }; | |
| } | |
| private static getNotifierOptions(env: Environment): NotifierConfig<NotifierProvider> { | |
| if (env.NOTIFIER_PROVIDER === "slack") { | |
| if (!env.SLACK_WEBHOOK_URL) { | |
| throw new Error("SLACK_WEBHOOK_URL is required when NOTIFIER_PROVIDER is 'slack'"); | |
| } | |
| return { | |
| notifierProvider: env.NOTIFIER_PROVIDER, | |
| opts: { | |
| webhookUrl: env.SLACK_WEBHOOK_URL, | |
| }, | |
| }; | |
| } | |
| return { | |
| notifierProvider: "null", | |
| }; | |
| } |
There was a problem hiding this comment.
🧩 Analysis chain
@0xnigir1 Thank you for implementing the suggested improvements! The changes will make the error handling more robust and the code more maintainable.
🏁 Script executed:
#!/bin/bash
# Fetch the specific commit to verify changes
gh pr view 66 --json commits --jq '.commits[] | select(.oid == "20123cb85315daf582d45b924ee2089b2a82a2e2") | .messageHeadline'
Length of output: 145
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| RETRY_MAX_DELAY_MS=300000 | ||
|
|
||
| NOTIFIER_PROVIDER=# 'slack' or 'null' |
There was a problem hiding this comment.
Now or later we should add these to the README and mark if required or not
There was a problem hiding this comment.
thanks for the reminder ser
packages/shared/README.md
Outdated
|
|
||
| #### Slack | ||
|
|
||
| Visit [Slack API docs](https://api.slack.com/messaging/webhooks) to create a Slack app and get a webhook URL. Follow the instructions for setting up a Slack app and get a webhook URL. Also, you can further customize the bot name, icons and scopes. |
There was a problem hiding this comment.
The first two sentences are repeats (:
| formatter?: (info: TransformableInfo) => IncomingWebhookSendArguments; | ||
| } | ||
|
|
||
| export class SlackNotifier implements INotifier { |
| warn: vi.fn(), | ||
| }; | ||
|
|
||
| it("creates SlackNotifier instance with valid Slack configuration", () => { |
There was a problem hiding this comment.
Might be good to test a failure scenario
There was a problem hiding this comment.
i've also added a validation for the URL but that test corresponds to the slack.notifier.spec.ts test suite
|
We should rename the PR title to be conventional commit syntax I think |
| import { IncomingWebhook, IncomingWebhookSendArguments } from "@slack/webhook"; | ||
|
|
||
| import { ILogger, stringify } from "../internal.js"; | ||
| import { INotifier, NotifierContext } from "./notifier.interface.js"; |
| @@ -0,0 +1,24 @@ | |||
| import { SlackOptions } from "./slack.notifier.js"; | |||
There was a problem hiding this comment.
Looks good
I noticed in the merge PR the indexer graphql var in .env.example has a leading white space now but that's not part of this PR so we can fix in another
https://github.com/defi-wonderland/grants-stack-indexer-v2/blob/dev/.env.example
🤖 Linear
Closes GIT-251 GIT-252
Description
We implement a notifier for sending notifications to a Slack channel using Slack Webhooks.
Also, we add a Null Notifier and a Notifier factory for creating the notifiers
Example message on Slack:
Checklist before requesting a review
Summary by CodeRabbit
New Features
Documentation