feat: alert template message pt3 #326
Conversation
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested.
- Reviewed the entire pull request up to b2bc9de
- Looked at
329lines of code in1files - Took 1 minute and 40 seconds to review
More info
- Skipped
2files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /packages/api/src/tasks/checkAlerts.ts:323:
- Assessed confidence :
100% - Grade:
0% - Comment:
The functionnotifyChannelis defined twice in this file. Please remove or rename this function to avoid confusion and unexpected behavior. - Reasoning:
The functionnotifyChannelis defined twice in the code. The first definition is on line 134 and the second one is on line 323. This is a clear violation of best practices as it can lead to confusion and unexpected behavior. The second definition of the function should be removed or renamed if it serves a different purpose.
Workflow ID: wflow_5qcz5E4t0x195EiO
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
wrn14897
force-pushed
the
warren/alert-template-message-pt3
branch
from
b2bc9de
to
8245c93
Compare
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 8245c93
- Looked at
24lines of code in1files - Took 2 minutes and 46 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of50%.
Workflow ID: wflow_954305eVcfShfnKY
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 7a99b4f
- Looked at
74lines of code in1files - Took 1 minute and 51 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
3additional comments because they didn't meet confidence threshold of50%.
1. /packages/api/src/tasks/checkAlerts.ts:269:
- Assessed confidence :
80% - Grade:
20% - Comment:
Consider renaming the functionrenderAlertTemplateto something likebuildAndSendAlertMessageto better reflect its purpose of building the alert message body and sending the alert to the channel. - Reasoning:
The functionrenderAlertTemplateis being used to build the body of the alert message and send the alert to the channel. However, the function name does not reflect this functionality. It would be more appropriate to name it something likebuildAndSendAlertMessageto better reflect its purpose.
2. /packages/api/src/tasks/checkAlerts.ts:170:
- Assessed confidence :
90% - Grade:
0% - Comment:
Consider handling unsupported channel types more gracefully in thenotifyChannelfunction. Instead of throwing an error, you could log a warning and skip the unsupported channel. This would prevent the program flow from being disrupted by an error. - Reasoning:
The functionnotifyChannelis throwing an error for unsupported channel types. However, it would be better to handle this gracefully, perhaps by logging a warning and skipping the unsupported channel, rather than throwing an error and potentially disrupting the flow of the program.
3. /packages/api/src/tasks/checkAlerts.ts:217:
- Assessed confidence :
90% - Grade:
40% - Comment:
Consider creating a single instance of Handlebars and reusing it, rather than creating a new instance every time thebuildAlertMessageTemplateTitlefunction is called. This could improve performance if the function is called frequently. - Reasoning:
The functionbuildAlertMessageTemplateTitleis creating a new instance of Handlebars every time it is called. This could be inefficient if the function is called frequently. It would be better to create a single instance of Handlebars and reuse it.
Workflow ID: wflow_hVUSsUWcdAKbFVqB
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 87f97eb
- Looked at
26lines of code in2files - Took 1 minute and 6 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /packages/api/src/tasks/checkAlerts.ts:295:
- Assessed confidence :
0% - Comment:
The addition of.trim()here will remove leading and trailing whitespace from the template string. This seems to match the intent suggested by the change in the test file. - Reasoning:
The change in the test file is removing an empty string from the expected output. This suggests that the change in the main file is intended to remove an extra newline from the output. The change in the main file is adding a.trim()call to the end of a template string. This will remove leading and trailing whitespace, including newlines, from the string. This seems to match the intent suggested by the test change.
Workflow ID: wflow_zz2wMxYmhODLQtgu
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on d09cb7f
- Looked at
13lines of code in1files - Took 1 minute and 18 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /packages/api/src/tasks/__tests__/checkAlerts.test.ts:89:
- Assessed confidence :
0% - Comment:
Good catch on removing the.onlyfrom the test suite. This ensures all tests are being run, which is crucial in a shared codebase. - Reasoning:
The change in this PR is a simple removal of the 'only' keyword from a test suite. This is a good change because 'only' restricts the test runner to only run the specified suite or test, which is not ideal in a shared codebase where we want to run all tests.
Workflow ID: wflow_X9vbt6BANySicDaN
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 2ae9099
- Looked at
312lines of code in2files - Took 2 minutes and 58 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of50%.
Workflow ID: wflow_mtOPENanOCqvW1xm
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
| switch (channel) { | ||
| case 'slack_webhook': { | ||
| const webhook = await Webhook.findOne({ | ||
| _id: id, |
There was a problem hiding this comment.
This function should handle the case where the webhook is not found in the database. Consider adding a check for whether the webhook is null and handle it appropriately.
There was a problem hiding this comment.
hmm...it seems AI is not there yet
There was a problem hiding this comment.
Cofounder of Ellipsis here, thanks for this feedback!
There was a problem hiding this comment.
hey @hbrooks , what a surprise! Thanks for checking it out
| } else if (alert.source === 'chart') { | ||
| if (dashboard == null) { | ||
| throw new Error('Source is CHART but dashboard is null'); | ||
| } | ||
| return `${group ? `Group: "${group}"` : ''} | ||
| rawTemplateBody = `${group ? `Group: "${group}"` : ''} | ||
| ${value} ${ | ||
| doesExceedThreshold( | ||
| alert.threshold_type === 'above', | ||
| alert.threshold, |
There was a problem hiding this comment.
Consider simplifying this complex ternary operation. You could create a separate function to handle this logic or use a more straightforward control flow structure like if-else statements.
| ? alert.threshold_type === 'above' | ||
| ? 'exceeds' | ||
| : 'falls below' | ||
| : alert.threshold_type === 'above' | ||
| ? 'falls below' | ||
| : 'exceeds' |
There was a problem hiding this comment.
found this existing bug 😅
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 202824d
- Looked at
82lines of code in2files - Took 2 minutes and 8 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
2additional comments because they didn't meet confidence threshold of50%.
1. /packages/api/src/tasks/checkAlerts.ts:275:
- Assessed confidence :
80% - Grade:
30% - Comment:
Consider implementing the sanity check for theidin thetranslateExternalActionsToInternalfunction. This will ensure theidconforms to the expected format and doesn't contain any malicious or problematic characters. - Reasoning:
ThetranslateExternalActionsToInternalfunction incheckAlerts.tshas been updated to handleidwith special characters and multiple dashes. This is a good improvement as it provides flexibility in the naming of theid. However, there is a comment about a sanity check that is marked as a TODO. It's important to validate theidto ensure it conforms to the expected format and doesn't contain any malicious or problematic characters.
2. /packages/api/src/tasks/checkAlerts.ts:281:
- Assessed confidence :
80% - Grade:
0% - Comment:
Consider including the actual value ofalert.sourcein the error message. This will provide more context when debugging issues related to therenderAlertTemplatefunction. - Reasoning:
TherenderAlertTemplatefunction incheckAlerts.tshas been updated to support rendering of alert templates. This is a good improvement as it provides flexibility in the creation of alert messages. However, there is a potential issue with the error handling. When thealert.sourceis not 'search' or 'chart', an error is thrown. However, the error message does not provide enough context about the actual value ofalert.sourcethat caused the error. Including the actual value in the error message would make debugging easier.
Workflow ID: wflow_ghdFzB1wpqOszMJz
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
| }) => { | ||
| switch (channel) { | ||
| case 'slack_webhook': { | ||
| const webhook = await Webhook.findOne({ |
There was a problem hiding this comment.
Consider adding error handling to manage scenarios where the id provided does not exist in the database. This will improve the robustness of the notifyChannel function.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 67cba25
- Looked at
76lines of code in2files - Took 1 minute and 54 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of50%.
Workflow ID: wflow_GjwDJbSFlz8jOhSK
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
wrn14897
force-pushed
the
warren/alert-template-message-pt3
branch
from
4969f09
to
bee519a
Compare
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 4969f09
- Looked at
22lines of code in1files - Took 1 minute and 27 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of50%.
1. /packages/api/src/tasks/checkAlerts.ts:158:
- Assessed confidence :
100% - Grade:
0% - Comment:
The regular expression used to match the prefix is not anchored to the start of the string. This could lead to unexpected matches. Consider adding^at the start of the regular expression to ensure it only matches at the beginning of the string.
$regex: new RegExp(`^${escapeRegExp(id)}`)
- Reasoning:
The regular expression used to match the prefix in thenotifyChannelfunction is not anchored to the start of the string. This could lead to unexpected matches.
Workflow ID: wflow_sgpYp1BDs2ErkPlZ
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on bee519a
- Looked at
670lines of code in3files - Took 7 minutes and 30 seconds to review
More info
- Skipped
2files when reviewing. - Skipped posting
3additional comments because they didn't meet confidence threshold of50%.
1. packages/api/src/tasks/checkAlerts.ts:135:
- Assessed confidence :
50% - Comment:
This function currently only supports Slack webhooks. Consider refactoring it to be more extensible and support other types of webhooks in the future. - Reasoning:
ThenotifyChannelfunction incheckAlerts.tsis currently only supporting Slack webhooks. This might limit the functionality of the system if other types of webhooks need to be supported in the future. It would be better to design this function in a more extensible way, so that it can easily support other types of webhooks.
2. packages/api/src/tasks/checkAlerts.ts:274:
- Assessed confidence :
80% - Grade:
20% - Comment:
Consider adding a sanity check or validation for the input to ensure it matches the expected format. - Reasoning:
ThetranslateExternalActionsToInternalfunction incheckAlerts.tsuses a regular expression to replace certain patterns in the template string. However, there is a comment indicating that a sanity check might be needed. This could potentially lead to unexpected behavior if the input does not match the expected format.
3. packages/api/src/tasks/checkAlerts.ts:286:
- Assessed confidence :
90% - Grade:
0% - Comment:
This function is quite long and contains a large if-else block. Consider breaking it down into smaller, more manageable functions to improve readability and maintainability. - Reasoning:
TherenderAlertTemplatefunction incheckAlerts.tshas a large block of code within an if-else statement. This makes the function quite long and a bit hard to read. It might be beneficial to break this function down into smaller, more manageable functions.
Workflow ID: wflow_Ko0fZnTrvTHYSkCq
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
| @@ -177,40 +225,71 @@ export const buildAlertMessageTemplateTitle = ({ | |||
| view: AlertMessageTemplateDefaultView; | |||
| }) => { | |||
| const { alert, dashboard, savedSearch, value } = view; | |||
| const handlebars = Handlebars.create(); | |||
There was a problem hiding this comment.
Consider refactoring this complex ternary operation to improve code readability.
wrn14897
force-pushed
the
warren/alert-template-message-pt3
branch
from
bee519a
to
d3e4c00
Compare
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on d3e4c00
- Looked at
670lines of code in3files - Took 2 minutes and 51 seconds to review
More info
- Skipped
2files when reviewing. - Skipped posting
4additional comments because they didn't meet confidence threshold of50%.
1. /packages/api/src/tasks/checkAlerts.ts:135:
- Assessed confidence :
50% - Comment:
Consider adding a comment to indicate that other types of channels will be supported in the future. - Reasoning:
ThenotifyChannelfunction is currently only supporting slack_webhook. It would be better to add a comment to indicate that other types of channels will be supported in the future.
2. /packages/api/src/tasks/checkAlerts.ts:274:
- Assessed confidence :
60% - Grade:
0% - Comment:
Consider adding a comment to explain what this regular expression does for better readability. - Reasoning:
ThetranslateExternalActionsToInternalfunction is using a regular expression to replace the@syntax with a template string. This is a good approach, but it would be better to add a comment explaining what the regular expression does for better readability.
3. /packages/api/src/tasks/checkAlerts.ts:285:
- Assessed confidence :
90% - Grade:
0% - Comment:
Consider breaking down this function into smaller, more manageable functions. - Reasoning:
TherenderAlertTemplatefunction is quite large and does a lot of things. It would be better to break it down into smaller, more manageable functions.
4. /packages/api/src/tasks/checkAlerts.ts:477:
- Assessed confidence :
90% - Grade:
20% - Comment:
Consider breaking down therenderAlertTemplatefunction into smaller, more manageable functions. - Reasoning:
ThefireChannelEventfunction is calling therenderAlertTemplatefunction, which is a large function that does a lot of things. It would be better to break down therenderAlertTemplatefunction into smaller, more manageable functions.
Workflow ID: wflow_GkMyAeEsgySGQhYz
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on f59ce35
- Looked at
670lines of code in3files - Took 2 minutes and 32 seconds to review
More info
- Skipped
2files when reviewing. - Skipped posting
2additional comments because they didn't meet confidence threshold of50%.
1. /packages/api/src/tasks/checkAlerts.ts:135:
- Assessed confidence :
80% - Grade:
20% - Comment:
Consider refactoring thenotifyChannelfunction to be more extensible for supporting more types of channels in the future. - Reasoning:
ThenotifyChannelfunction is currently only supporting slack_webhook. It would be better to have a more extensible design to support more types of channels in the future.
2. /packages/api/src/tasks/checkAlerts.ts:285:
- Assessed confidence :
90% - Grade:
40% - Comment:
Consider refactoring therenderAlertTemplatefunction to break it down into smaller, more manageable functions. - Reasoning:
TherenderAlertTemplatefunction is quite large and complex. It might be beneficial to break it down into smaller, more manageable functions.
Workflow ID: wflow_TCu3ZpaBgx6z0OtC
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Use Handlebars to build up alert template and trigger notifyChannel method (with
@prefix)Summary:
This PR enhances the alert template building process, supports slack_webhook in the notifyChannel method, introduces new functions, removes the
extractChannelsfunction, updates thecheckAlerts.test.tsfile, all within thecheckAlerts.tsfile, and modifies thewebhook.tsmodel to include a WebhookService enum and updates the schema.Key points:
checkAlerts.tscheckAlerts.tscheckAlerts.tsextractChannelsfunction incheckAlerts.tscheckAlerts.test.tswebhook.tsmodel to include a WebhookService enum and updates the schemaGenerated with ❤️ by ellipsis.dev