-
Notifications
You must be signed in to change notification settings - Fork 3
feat: slack notifications #66
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
Changes from 3 commits
1aaa32f
6a1656f
a4172b2
e839723
20123cb
9431c30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -21,7 +21,16 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
KyselyStrategyRegistryRepository, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
KyselyTransactionManager, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} from "@grants-stack-indexer/repository"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { ExponentialBackoff, ILogger, Logger, RetryStrategy } from "@grants-stack-indexer/shared"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ExponentialBackoff, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ILogger, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
INotifier, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Logger, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
NotifierConfig, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
NotifierFactory, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
NotifierProvider, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
RetryStrategy, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} from "@grants-stack-indexer/shared"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { Environment } from "../config/index.js"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -36,6 +45,7 @@ export type SharedDependencies = { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kyselyDatabase: ReturnType<typeof createKyselyDatabase>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
retryStrategy: RetryStrategy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logger: ILogger; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
notifier: INotifier; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -48,6 +58,8 @@ export class SharedDependenciesService { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
static async initialize(env: Environment): Promise<SharedDependencies> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const logger = Logger.getInstance(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const notifier = NotifierFactory.create(this.getNotifierOptions(env), logger); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// Initialize repositories | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const kyselyDatabase = createKyselyDatabase( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -146,6 +158,22 @@ export class SharedDependenciesService { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
kyselyDatabase, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
retryStrategy, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logger, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
notifier, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix static method reference and enhance error handling. The static method has issues with 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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:
Length of output: 145 If you found this review helpful, would you consider giving us a shout-out on X? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,3 +39,13 @@ You can import the package in your TypeScript files as follows: | |
```typescript | ||
import { ILogger, ProcessorEvent, stringify } from "@grants-stack-indexer/shared"; | ||
``` | ||
|
||
### Configuring the Notifier | ||
|
||
#### 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first two sentences are repeats (: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
#### Null | ||
|
||
The null notifier is a no-op notifier that does nothing. It is useful when you want to disable notifications. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
export * from "./notifier.interface.js"; | ||
export * from "./notifierConfig.interface.js"; | ||
export * from "./slack.notifier.js"; | ||
export * from "./null.notifier.js"; | ||
export * from "./notifier.factory.js"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import { ILogger, NullNotifier, SlackNotifier } from "../internal.js"; | ||
import { INotifier } from "./notifier.interface.js"; | ||
import { NotifierConfig, NotifierProvider } from "./notifierConfig.interface.js"; | ||
|
||
/** | ||
* Factory class to create instances of INotifier based on configuration. | ||
*/ | ||
export class NotifierFactory { | ||
/** | ||
* Creates an instance of INotifier based on configuration. | ||
* | ||
* @param {NotifierConfig<NotifierProvider>} options - The configuration for the notifier. | ||
* @returns {INotifier} An instance of INotifier. | ||
*/ | ||
static create(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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,15 @@ | ||||||||||||||||||||||||||||||||||
import { AnyIndexerFetchedEvent, ChainId } from "../internal.js"; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
export type NotifierContext = { | ||||||||||||||||||||||||||||||||||
event?: AnyIndexerFetchedEvent; | ||||||||||||||||||||||||||||||||||
chainId: ChainId; | ||||||||||||||||||||||||||||||||||
stack?: string; | ||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||
Comment on lines
+9
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@0xnigir1 Thank you for implementing the suggested changes! The JSDoc documentation has been added correctly. 🏁 Script executed:
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 If you found this review helpful, would you consider giving us a shout-out on X? |
||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@0xnigir1 Thank you for implementing the JSDoc improvements! The documentation is now more comprehensive and clearer. (`∇´ノ)ノ 🐰✨ 🏁 Script executed:
Length of output: 2379 @0xnigir1 Excellent work! Not only did you implement the suggested JSDoc improvements for the (◕ᴗ◕✿) 🐰 If you found this review helpful, would you consider giving us a shout-out on X? |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,14 @@ | ||||||||||||||||||||||||||||||||
import { SlackOptions } from "./slack.notifier.js"; | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. improts from index There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
export type NotifierProvider = "slack" | "null"; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
export type SlackNotifierConfig = { | ||||||||||||||||||||||||||||||||
notifierProvider: "slack"; | ||||||||||||||||||||||||||||||||
opts: SlackOptions; | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
Comment on lines
+10
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@0xnigir1 Perfect! The JSDoc documentation has been added correctly. This will help improve code maintainability and developer experience. 🏁 Script executed:
Length of output: 1009 @0xnigir1 Excellent work! Not only did you add the suggested documentation for If you found this review helpful, would you consider giving us a shout-out on X? |
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
export type NotifierConfig<Source extends NotifierProvider> = Source extends "slack" | ||||||||||||||||||||||||||||||||
? SlackNotifierConfig | ||||||||||||||||||||||||||||||||
: { | ||||||||||||||||||||||||||||||||
notifierProvider: "null"; | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
Comment on lines
+20
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain@0xnigir1 Thank you for implementing the suggested changes! The JSDoc documentation has been added correctly. 🏁 Script executed:
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 If you found this review helpful, would you consider giving us a shout-out on X? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import { INotifier, NotifierContext } from "../internal.js"; | ||
|
||
/** | ||
* A null implementation of INotifier that performs no actions but allows the application to have an undefined notifier if desired | ||
*/ | ||
export class NullNotifier implements INotifier { | ||
async send(_message: string, _context: NotifierContext): Promise<void> { | ||
// Do nothing | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now or later we should add these to the README and mark if required or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the reminder ser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
20123cb