feat: add triggers ids support ENG-2903 #1038
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update enhances the trigger fetching functionality by allowing retrieval using specific trigger IDs. It introduces input validation with Zod, ensuring that at least one parameter (appNames, triggerIds, connectedAccountsIds, integrationIds) is provided. A new test case verifies the retrieval by trigger ID, enhancing the feature's robustness and reliability. Changes
InstructionsEmoji Descriptions:
Interact with the Bot:
Execute a command using the format:
Available Commands:
Tips for Using @bot Effectively:
Need More Help?📚 Visit our documentation for detailed guides on using Entelligence.AI. |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
js/src/sdk/models/triggers.spec.ts
Outdated
| expect(trigger.status).toBe("success"); | ||
| }); | ||
|
|
||
|
|
||
| it("should retrieve a list of triggers for a specific triggerId", async () => { | ||
| const triggerList = await triggers.list({ | ||
| triggerIds: ["GMAIL_NEW_GMAIL_MESSAGE"], | ||
| }); | ||
| expect(triggerList.length).toBeGreaterThan(0); | ||
| expect(triggerList[0].name).toBe("GMAIL_NEW_GMAIL_MESSAGE"); | ||
| }); | ||
|
|
||
| // it("should subscribe to a trigger and receive a trigger", async () => { | ||
| // function waitForTriggerReceived() { |
There was a problem hiding this comment.
Review of New Test Case for Trigger Retrieval by ID
The newly added test case for retrieving a list of triggers by specific trigger IDs is a crucial addition, as it tests a core feature of the system. Here are some actionable suggestions to ensure its correctness and completeness:
- Verify Edge Cases: Ensure that the test case covers scenarios where the
triggerIdsarray is empty, contains invalid IDs, or includes a mix of valid and invalid IDs. This will help confirm that the function handles these situations gracefully. - Assertions: While the current assertions check for the length and name of the first trigger, consider adding checks for other properties of the trigger object to ensure comprehensive validation.
- Error Handling: Include tests to verify the behavior when the API call fails or returns an error. This will ensure robustness in error scenarios.
By addressing these points, the test case will be more robust and reliable, ensuring that the feature works as intended across various scenarios. 🛠️
🔧 Suggested Code Diff:
+ it("should handle empty triggerIds array", async () => {
+ const triggerList = await triggers.list({ triggerIds: [] });
+ expect(triggerList.length).toBe(0);
+ });
+
+ it("should handle invalid triggerIds", async () => {
+ const triggerList = await triggers.list({ triggerIds: ["INVALID_ID"] });
+ expect(triggerList.length).toBe(0);
+ });
+
+ it("should handle mix of valid and invalid triggerIds", async () => {
+ const triggerList = await triggers.list({ triggerIds: ["GMAIL_NEW_GMAIL_MESSAGE", "INVALID_ID"] });
+ expect(triggerList.length).toBe(1);
+ expect(triggerList[0].name).toBe("GMAIL_NEW_GMAIL_MESSAGE");
+ });📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| expect(trigger.status).toBe("success"); | |
| }); | |
| it("should retrieve a list of triggers for a specific triggerId", async () => { | |
| const triggerList = await triggers.list({ | |
| triggerIds: ["GMAIL_NEW_GMAIL_MESSAGE"], | |
| }); | |
| expect(triggerList.length).toBeGreaterThan(0); | |
| expect(triggerList[0].name).toBe("GMAIL_NEW_GMAIL_MESSAGE"); | |
| }); | |
| // it("should subscribe to a trigger and receive a trigger", async () => { | |
| // function waitForTriggerReceived() { | |
| it("should retrieve a list of triggers for a specific triggerId", async () => { | |
| const triggerList = await triggers.list({ | |
| triggerIds: ["GMAIL_NEW_GMAIL_MESSAGE"], | |
| }); | |
| expect(triggerList.length).toBeGreaterThan(0); | |
| expect(triggerList[0].name).toBe("GMAIL_NEW_GMAIL_MESSAGE"); | |
| // Additional assertions for other properties can be added here | |
| }); | |
| it("should handle empty triggerIds array", async () => { | |
| const triggerList = await triggers.list({ triggerIds: [] }); | |
| expect(triggerList.length).toBe(0); | |
| }); | |
| it("should handle invalid triggerIds", async () => { | |
| const triggerList = await triggers.list({ triggerIds: ["INVALID_ID"] }); | |
| expect(triggerList.length).toBe(0); | |
| }); | |
| it("should handle mix of valid and invalid triggerIds", async () => { | |
| const triggerList = await triggers.list({ triggerIds: ["GMAIL_NEW_GMAIL_MESSAGE", "INVALID_ID"] }); | |
| expect(triggerList.length).toBe(1); | |
| expect(triggerList[0].name).toBe("GMAIL_NEW_GMAIL_MESSAGE"); | |
| }); |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7027ecc in 21 seconds
More details
- Looked at
49lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_hVnV9Iomwif9UzQT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 1786e62 in 1 minute and 2 seconds
More details
- Looked at
100lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/triggers.spec.ts:98
- Draft comment:
Consider updating the test description to "should retrieve a list of triggers and verify the first trigger's name for a specific triggerId" for clarity. - Reason this comment was not posted:
Confidence changes required:50%
The test case for retrieving triggers bytriggerIdis correctly implemented. However, the test description could be more precise by specifying that it checks for a specific trigger name.
Workflow ID: wflow_tG2sK8CW9dUan9Hb
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| const ZTriggerQuery = z.object({ | ||
| triggerIds: z.array(z.string()).optional(), | ||
| appNames: z.array(z.string()).optional(), | ||
| connectedAccountsIds: z.array(z.string()).optional(), |
There was a problem hiding this comment.
Typo in connectedAccountsIds. It should be connectedAccountIds to match the expected query parameter in the API call.
js/src/sdk/models/triggers.ts
Outdated
| TELEMETRY_LOGGER.manualTelemetry(TELEMETRY_EVENTS.SDK_METHOD_INVOKED, { | ||
| method: "list", | ||
| file: this.fileName, | ||
| params: { data }, | ||
| }); | ||
| const {appNames, triggerIds, connectedAccountsIds, integrationIds, showEnabledOnly} = ZTriggerQuery.parse(data); | ||
|
|
||
| if(!appNames && !triggerIds && !connectedAccountsIds && !integrationIds) { |
There was a problem hiding this comment.
Consider adding validation for empty arrays as well. Currently, passing an empty array would pass this validation but might not be a valid use case. You could add:
const hasValidFilter = (arr?: string[]) => arr && arr.length > 0;
if (!hasValidFilter(appNames) && !hasValidFilter(triggerIds) &&
!hasValidFilter(connectedAccountsIds) && !hasValidFilter(integrationIds)) {
throw CEG.getCustomError(...);
}
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
| @@ -29,18 +40,31 @@ export class Triggers { | |||
| * @returns {CancelablePromise<ListTriggersResponse>} A promise that resolves to the list of all triggers. | |||
| * @throws {ApiError} If the request fails. | |||
| */ | |||
| async list(data: RequiredQuery = {}) { | |||
| async list(data: TTriggerQuery = {}) { | |||
There was a problem hiding this comment.
The JSDoc comment for the list method should be updated to reflect the new parameters and validation requirements. Consider adding:
/**
* Retrieves a list of triggers based on provided filters.
* @param {TTriggerQuery} data Query parameters for filtering triggers
* @param {string[]} [data.triggerIds] - Filter by specific trigger IDs
* @param {string[]} [data.appNames] - Filter by app names
* @param {string[]} [data.connectedAccountsIds] - Filter by connected account IDs
* @param {string[]} [data.integrationIds] - Filter by integration IDs
* @param {boolean} [data.showEnabledOnly] - Show only enabled triggers
* @throws {CustomError} When no valid filter parameters are provided
* @returns {Promise<Array>} List of matching triggers
*/
Code Review SummaryOverall AssessmentThe changes look good and add valuable functionality for filtering triggers by IDs. The implementation is clean and follows existing patterns. Positive Aspects✅ Good use of Zod for input validation Suggestions for Improvement
Code Quality: 8/10The implementation is solid with good error handling and validation. The suggestions above are minor improvements that would make the code more robust and maintainable. Good to merge after addressing the comments! 👍 |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1dc542a in 32 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/triggers.ts:55
- Draft comment:
Removing the validation check for at least one parameter (appNames, triggerIds, connectedAccountsIds, integrationIds) being required could lead to issues if the API expects at least one of these parameters. Consider re-adding this validation to prevent potential errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The validation seems to have been intentionally removed. The schema explicitly marks all fields as optional, and the API call handles null/undefined values gracefully. Without seeing the API implementation, we can't be certain if the API truly requires one of these parameters. The removal of this check suggests the API likely supports calls without any of these parameters.
I could be wrong about the API's requirements - maybe it does need at least one parameter but handles the error differently now. The schema being optional doesn't guarantee the API accepts empty queries.
However, the code change was intentional and the type system + schema explicitly support all parameters being optional. Without clear evidence that the API requires parameters, we should trust the author's change.
The comment should be deleted as it's speculative without strong evidence that the API requires parameters, and it violates our rule about not making speculative comments.
Workflow ID: wflow_pOLFKcor4N67bT8f
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
himanshu-dixit
changed the title
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c382661 in 31 seconds
More details
- Looked at
28lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/triggers.ts:51
- Draft comment:
Move the destructuring ofZTriggerQuery.parse(data)outside the try block to ensure validation errors are not caught by the catch block. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment has a valid point - validation errors are different from API errors and might deserve different handling. However, looking at the code pattern across the file, all operations including validations are consistently wrapped in try-catch with CEG.handleAllError. This suggests the error handling is intentionally designed to handle all types of errors, including validation errors, in a consistent way.
I might be missing something about the CEG.handleAllError implementation - perhaps it doesn't handle validation errors well? The consistent pattern across the file could be a mistake rather than intentional design.
Without seeing the implementation of CEG.handleAllError, and given the consistent pattern across the codebase, it's safer to assume the current error handling is intentional and works as designed.
The comment should be deleted as it suggests changing an apparently intentional error handling pattern without strong evidence that the current approach is problematic.
Workflow ID: wflow_z49NAOMjFQYWMVP2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 103dea6 in 12 seconds
More details
- Looked at
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/triggers.spec.ts:33
- Draft comment:
Good use of a variable for the app name to improve maintainability and readability of the test. - Reason this comment was not posted:
Confidence changes required:0%
The test case for retrieving triggers by app name was updated to use a variable for the app name, which is a good practice for maintainability.
Workflow ID: wflow_46KsyJ7by6ydqvQo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 06fd71d in 32 seconds
More details
- Looked at
63lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/triggers.spec.ts:112
- Draft comment:
The error message in the test does not match the error message in the code. The test expects "No triggers found for the given filters", but the code throws "No triggers match the specified filters". Update the test to match the actual error message. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Without seeing the actual error message in the code, I can't verify if there's really a mismatch. The comment makes an assertion about code we can't see. Following the principle of needing strong evidence, we should err on the side of removing comments when we can't verify their accuracy.
I might be missing the error message definition in another file. The SDK_ERROR_CODES import could be relevant to this.
Even if the error message is defined elsewhere, we can't verify the comment's accuracy without seeing it. We should follow the principle of removing comments when we lack strong evidence.
Delete the comment because we cannot verify its accuracy without seeing the actual error message definition.
Workflow ID: wflow_rLF8XMEAOj8iYcU0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 360b0bb in 15 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/triggers.ts:5
- Draft comment:
Consider organizing imports by grouping them logically, such as third-party libraries first, followed by internal modules. This improves readability and maintainability. - Reason this comment was not posted:
Confidence changes required:20%
The import order is not consistent with best practices. It's better to group imports by their origin and purpose.
Workflow ID: wflow_xinrvXVpAbiow8Mm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
🔍 Review Summary
Purpose
Enhance the trigger fetching functionality by allowing retrieval using specific trigger IDs and ensure input validation with Zod for at least one parameter.
Changes
New Feature
Enhancement
Bug Fix
Test
Impact
Empowers users with enhanced trigger fetching functionality which allows the retrieval of specific trigger IDs. Validations have been improved and the robustness of retrieval function is increased due to added test cases.
Original Description
No existing description found
Important
Add support for fetching triggers by IDs and enhance input validation with Zod in
triggers.ts.list()method intriggers.ts.ZTriggerQueryfor input validation intriggers.ts.list()method intriggers.ts.triggerIdsintriggers.spec.ts.appNamesintriggers.spec.ts.This description was created by
for 360b0bb. It will automatically update as commits are pushed.