feat: SQS triggers #5182
Merged
feat: SQS triggers #5182
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… into feat/sqs-triggers
rubenfiszel
force-pushed
the
main
branch
2 times, most recently
from
3069a7d to
0b0e564
Compare
Deploying windmill with
|
| Latest commit: |
2d4a292
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b26a4195.windmill.pages.dev |
| Branch Preview URL: | https://dieri-sqs-trigger.windmill.pages.dev |
![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 everything up to 73f51a2 in 2 minutes and 28 seconds
More details
- Looked at
3222lines of code in44files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. frontend/src/lib/components/triggers/CaptureButton.svelte:85
- Draft comment:
SQS trigger option uses the Unplug icon; consider using a dedicated AWS icon (e.g. AwsIcon) for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the suggestion for consistency makes some sense since SQS is an AWS service, this seems like a minor UI detail. The comment doesn't point to any functional issues. There's no clear evidence that using an AWS icon is required or that the current icon choice is problematic. Other trigger types also use generic icons (like Route for HTTP).
The comment could be valid from a UX perspective as SQS is specifically an AWS service. The current icon might be confusing to users.
However, this is a pure UI styling choice without functional impact. Per the rules, we should not comment on UI/styling changes as we should assume the author made these choices deliberately.
Delete the comment as it relates to purely UI/styling choices which are outside the scope of what we should comment on according to the rules.
2. frontend/src/lib/components/triggers/sqs/QueueSetup.svelte:20
- Draft comment:
URL validation regex only accepts URLs starting with 'https://'. Confirm if this is intended for SQS queues (typically SQS endpoints use https). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is asking the author to confirm their intention, which violates our rules. AWS SQS queues do use HTTPS URLs, so this validation is correct. Even if it wasn't, asking for confirmation isn't actionable - the comment should either point out a specific issue or be removed.
Maybe there are some edge cases where SQS URLs use a different protocol that we're not aware of? Maybe this validation is too strict?
Even if there were edge cases, the comment isn't pointing them out - it's just asking for confirmation. If there was a specific issue, the comment should state it directly.
Delete this comment. It violates our rule against asking authors to confirm their intentions, and doesn't point out any actual issues.
3. frontend/src/lib/components/triggers/CaptureButton.svelte:19
- Draft comment:
Ensure that the new 'sqs' case in handleClick is correctly mapped in captureTriggerKindToTriggerKind. Check consistency with triggers.ts definition. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/lib/components/triggers.ts:68
- Draft comment:
SQS trigger kind is added to TriggerKind and captureTriggerKindToTriggerKind; verify that all services handling triggers (e.g. count, update) correctly support 'sqs' in backend integration. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to verify that all services handling triggers support the new 'sqs' trigger kind. This falls under asking the author to ensure the behavior is intended or to double-check things, which is against the rules.
5. frontend/src/lib/script_helpers.ts:581
- Draft comment:
New preprocessor module for SQS triggers added; review that security and input sanitization are adequate. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. frontend/src/lib/script_helpers.ts:978
- Draft comment:
The function isInitialCode uses nested forEach with a return inside the callback. This won’t break out of the loops, so the function always returns false. Use Array.prototype.some or a for–of loop instead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/lib/components/triggers/CaptureButton.svelte:86
- Draft comment:
For the Sqs trigger button, the icon is currently set to . Consider using an AWS‐specific icon (like AwsIcon) for better consistency and clarity. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_4GdNfwLSvYnlHOfE
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.
| dirtyPath = false | ||
| await loadTrigger() | ||
| } catch (err) { | ||
| sendUserToast(`Could not load postgres trigger: ${err.body}`, true) |
There was a problem hiding this comment.
Error message mentions 'postgres trigger'—should be updated to 'Sqs trigger'.
| eventStreamType = $selectedTrigger | ||
| } | ||
| } | ||
|
|
||
| const { selectedTrigger, simplifiedPoll } = getContext<TriggerContext>('TriggerContext') | ||
|
|
||
| const dispatch = createEventDispatcher() | ||
|
|
||
| console.log({ selectedTrigger: $selectedTrigger, eventStreamType }) |
There was a problem hiding this comment.
Remove or disable the console.log debugging statement in production code.
| icon: Trash, | ||
| disabled: !canWrite, | ||
| action: async () => { | ||
| await SqsTriggerService.deleteSqsTrigger({ |
There was a problem hiding this comment.
When deleting a trigger in the Dropdown action, wrap the API call in try-catch to handle any errors and show appropriate user feedback.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2a6dee3 in 1 minute and 27 seconds
More details
- Looked at
91lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. frontend/src/lib/components/triggers/TriggersEditor.svelte:41
- Draft comment:
Good removal of the debug log. Ensure that if you ever need logging in development, it's behind a debug flag. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
2. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:50
- Draft comment:
Corrected error message from 'postgres trigger' to 'sqs trigger'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. frontend/src/routes/(root)/(logged)/sqs_triggers/+page.svelte:360
- Draft comment:
Good improvement adding try-catch around deleteSqsTrigger. This prevents unhandled errors and notifies the user appropriately. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. frontend/src/routes/(root)/(logged)/sqs_triggers/+page.svelte:139
- Draft comment:
Formatting changes in the filtering sections improve readability. No functional changes detected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. frontend/src/lib/components/triggers/TriggersEditor.svelte:41
- Draft comment:
Good removal of the debug log. Ensuring no stray console logs in production is best practice. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it praises the removal of a debug log and mentions a best practice without suggesting any changes or improvements. It doesn't ask for confirmation or suggest any specific action.
6. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:50
- Draft comment:
Corrected error message to reference 'sqs trigger' instead of 'postgres trigger'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is purely informative, as it only states what was changed without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest any improvements.
7. frontend/src/routes/(root)/(logged)/sqs_triggers/+page.svelte:140
- Draft comment:
Minor formatting adjustments in filter functions improve readability. Ensure these align with your project’s style settings. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is purely informative and suggests ensuring alignment with project style settings, which violates the rule against asking the PR author to ensure something. It doesn't provide a specific code suggestion or ask for a specific change.
8. frontend/src/routes/(root)/(logged)/sqs_triggers/+page.svelte:357
- Draft comment:
Wrapping deleteSqsTrigger call in a try/catch enhances error handling. Consider verifying that error.body exists or providing a fallback message. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment is about error handling which was changed in the diff. However, error handling through sendUserToast is used consistently throughout the codebase (seen in setTriggerEnabled). The utility function likely handles undefined error.body gracefully. Making defensive checks would add complexity without clear benefit.
I could be wrong about sendUserToast's error handling capabilities. There could be cases where error.body is undefined that cause issues.
While error.body could be undefined, the consistent use of sendUserToast throughout the codebase and its nature as a utility function suggests it handles error cases appropriately. Adding checks would be defensive programming without clear benefit.
The comment should be deleted as it suggests adding complexity without clear evidence of an actual issue. The error handling approach used is consistent with the codebase patterns.
Workflow ID: wflow_BnVS1rTDwLmPorqz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
rubenfiszel
force-pushed
the
main
branch
2 times, most recently
from
30f0807 to
9c57565
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 217b0fe in 2 minutes and 10 seconds
More details
- Looked at
182lines of code in5files - Skipped
0files when reviewing. - Skipped posting
12drafted comments based on config settings.
1. frontend/src/lib/components/triggers/http/RouteEditorInner.svelte:369
- Draft comment:
Repositioned RouteEditorConfigSection. Confirm that moving it below 'Target' is intentional and doesn't break layout or user flow. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. frontend/src/lib/components/triggers/kafka/KafkaTriggerEditorInner.svelte:236
- Draft comment:
Moved KafkaTriggersConfigSection below 'Runnable'. Ensure its new placement maintains the intended UX. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. frontend/src/lib/components/triggers/nats/NatsTriggerEditorInner.svelte:242
- Draft comment:
NatsTriggersConfigSection was moved. Verify that removing it from the top and re-inserting later doesn’t affect data binding. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:72
- Draft comment:
Default for message_attributes changed from [] to ['All']. Confirm this intentional default change and its downstream effects. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment violates several rules: 1) It asks for confirmation of intention which is explicitly forbidden 2) It's speculative about "downstream effects" 3) It doesn't point out a clear issue that needs fixing. The change from [] to ['All'] appears to be an intentional improvement to the default value.
Maybe this default change could cause issues with existing code that expects an empty array. The 'All' value might have special meaning that needs to be considered.
While those are valid concerns, the review rules clearly state we should not ask for confirmation or make speculative comments. If there were actual issues, they would be caught in testing.
This comment should be deleted as it violates the rules by asking for confirmation and making speculative statements without pointing out a clear issue that needs fixing.
5. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:230
- Draft comment:
Repositioned SqsTriggerEditorConfigSection appears at a different location. Ensure the configuration is still clearly associated with the related fields. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. frontend/src/lib/components/triggers/websocket/WebsocketTriggerEditorInner.svelte:309
- Draft comment:
WebsocketEditorConfigSection relocated. Verify that moving it to this new position doesn't affect initial URL or args binding. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
7. frontend/src/lib/components/triggers/http/RouteEditorInner.svelte:376
- Draft comment:
Reordered RouteEditorConfigSection from its original position. Ensure that moving this config section does not break form state, validation, or UI flow. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that moving the config section does not break form state, validation, or UI flow. This is asking the author to double-check their work, which violates the rules. The comment does not provide a specific suggestion or point out a specific issue.
8. frontend/src/lib/components/triggers/kafka/KafkaTriggerEditorInner.svelte:239
- Draft comment:
KafkaTriggersConfigSection is now moved below the Runnable section. Verify that state bindings (args, isValid) remain in sync and the new order supports the intended user flow. - Reason this comment was not posted:
Marked as duplicate.
9. frontend/src/lib/components/triggers/nats/NatsTriggerEditorInner.svelte:239
- Draft comment:
NatsTriggersConfigSection has been repositioned to follow the Runnable section. Confirm that this change does not inadvertently affect default values or UI behavior. - Reason this comment was not posted:
Marked as duplicate.
10. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:69
- Draft comment:
Default for message_attributes changed from [] to ['All']. Confirm that this default aligns with backend expectations and does not lead to unintended behavior. - Reason this comment was not posted:
Marked as duplicate.
11. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:235
- Draft comment:
SqsTriggerEditorConfigSection has been reinserted after the Runnable section. Ensure the order supports a logical UI flow and that bindings (isValid, queue_url, message_attributes, aws_resource_path) remain correct. - Reason this comment was not posted:
Marked as duplicate.
12. frontend/src/lib/components/triggers/websocket/WebsocketTriggerEditorInner.svelte:306
- Draft comment:
WebsocketEditorConfigSection is now repositioned after the initial messages section. Verify that the bindings (url, url_runnable_args, dirtyUrl, isValid) and overall user experience remain proper. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_oLWtp7eM7Zi5VoWU
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 91bd134 in 2 minutes and 5 seconds
More details
- Looked at
21lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:72
- Draft comment:
Changed default for message_attributes from ['All'] to []. Confirm that an empty array is the intended default as this may affect trigger behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This comment violates several rules. It's asking for confirmation ("Confirm that...") rather than pointing out a definite issue. It's speculative ("may affect trigger behavior"). If this was truly a breaking change, it would need to be fixed, not confirmed. The author likely intentionally made this change.
Maybe this is actually a serious breaking change that needs discussion? The 'All' value could have special meaning in SQS triggers.
Even if it is a breaking change, asking for confirmation is not helpful - either it needs to be fixed or it doesn't. The author intentionally made this change and would have tested it.
Delete this comment as it violates the rule against asking for confirmations and making speculative comments. If there was a real issue, it should be stated directly.
2. backend/ee-repo-ref.txt:1
- Draft comment:
Commit hash updated. Minor: file missing trailing newline. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:69
- Draft comment:
Default for message_attributes changed from ['All'] to []. Confirm if this behavior change is intentional. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_eutj8EocH5jboPCc
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
HugoCasa
previously approved these changes
Feb 14, 2025
rubenfiszel
force-pushed
the
main
branch
8 times, most recently
from
3a05600 to
3b6585a
Compare
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to c3f2b25 in 4 minutes and 56 seconds
More details
- Looked at
5308lines of code in95files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:56
- Draft comment:
Consider ensuring that defaultValues are properly null-checked before accessing properties to avoid runtime errors. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/lib/components/triggers/sqs/SqsTriggerPanel.svelte:40
- Draft comment:
Polling every 5 seconds with setInterval may not scale well if trigger list grows; review whether a push approach (e.g. websocket or SSE) would be more efficient. - Reason this comment was not posted:
Comment was on unchanged code.
3. frontend/src/lib/components/triggers/sqs/SqsTriggerPanel.svelte:240
- Draft comment:
When updating the owner's filter based on query parameters, consider debouncing to avoid excessive URL state updates during fast typing. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/lib/script_helpers.ts:1050
- Draft comment:
The isInitialCode function loops through all INIT_CODE values but uses a forEach with a 'return' inside which won't break the loop; consider using a proper iteration (e.g. a for loop) that can return immediately upon match. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:40
- Draft comment:
The openEdit function correctly opens the drawer and loads the trigger. Consider adding additional logging or a loading spinner during the asynchronous load to improve user feedback. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorConfigSection.svelte:30
- Draft comment:
The URL validation regex/^(https:)\/\/[^ \s]+$/is very basic. Consider reviewing the regex to handle edge cases (e.g. trailing spaces, valid URL characters) or use a dedicated URL parser for better validation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
SQS queue URLs have a very specific format. The current validation is intentionally basic since it only needs to verify https:// prefix. The error message shown to users is also simple, just saying "must start with https://". More complex validation would likely be handled server-side. The comment suggests a solution to a problem that may not exist.
I could be wrong about SQS URL formats - there might be specific requirements I'm not aware of. The basic validation could let through malformed URLs that cause issues.
Even if there are specific SQS URL format requirements, that validation would be better handled server-side where the actual connection is made. The frontend validation is appropriately simple for its purpose.
The comment should be deleted as it suggests adding complexity that isn't clearly needed for this specific use case of SQS queue URLs.
7. frontend/src/lib/components/triggers/sqs/SqsTriggerEditorInner.svelte:101
- Draft comment:
In updateTrigger, after creating/updating the trigger, an update event is dispatched and the drawer is closed. Consider checking if any further UI state (like resetting fields) is needed for consistency with other trigger components. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
8. frontend/src/lib/components/triggers/sqs/SqsTriggerPanel.svelte:48
- Draft comment:
Using setInterval for refreshing trigger status is acceptable; ensure that error cases are handled and consider if an exponential backoff is needed in high-load situations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
9. frontend/src/routes/(root)/(logged)/sqs_triggers/+page.svelte:227
- Draft comment:
The search and filter controls for SQS triggers follow patterns used in other trigger pages. Overall the integration appears consistent. Consider reusing shared filter components to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_17ltmuEy2nB7NQge
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.
| dirtyPath = false | ||
| await loadTrigger() | ||
| } catch (err) { | ||
| sendUserToast(`Could not load sqs trigger: ${err.body}`, true) |
There was a problem hiding this comment.
When catching errors in loadTrigger, use a null/undefined check before accessing err.body to avoid crashes if error is not structured.
Suggested change
| sendUserToast(`Could not load sqs trigger: ${err.body}`, true) | |
| sendUserToast(`Could not load sqs trigger: ${err.body ?? 'unknown error'}`, true) |
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
This pull request adds support for SQS triggers, including backend, frontend, and API changes to manage SQS triggers within the system.
backendwith new database migrations20250130184358_sqs_trigger.up.sqland20250130184358_sqs_trigger.down.sql.SqsTriggerServicewith CRUD operations.openapi.yamlto include SQS trigger endpoints.SqsTriggerEditor,SqsTriggerPanel, andSqsTriggerEditorConfigSectioncomponents for SQS trigger UI.+page.svelteand+layout.svelteto integrate SQS triggers into the UI.TriggersBadgeandSidebarContentto include SQS triggers.script_helpers.tsto include SQS in preprocessor module code.+page.sveltefor scripts and flows.aws-sdk-sqsdependency inCargo.toml.build-publish-rh-image.ymlanddocker-image.ymlto includesqs_triggerfeature.getUsedTriggersinworkspaces.tsto account for SQS triggers.This description was created by
for c3f2b25. It will automatically update as commits are pushed.