feat: native trigger #6797
Open
feat: native trigger #6797
+5,482
−181
Conversation
This file contains hidden or 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
dieriba
requested review from
alpetric,
hugocasa and
rubenfiszel
as code owners
|
Claude finished @dieriba's task —— View job Code Review for Native Trigger Feature
|
Deploying windmill with
|
| Latest commit: |
f721c5d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://99ffdc73.windmill.pages.dev |
| Branch Preview URL: | https://dieri-nextcloud-integration.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.
Caution
Changes requested ❌
Reviewed everything up to 0e20e35 in 2 minutes and 35 seconds. Click for details.
- Reviewed
4182lines of code in29files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/native_triggers/handler.rs:88
- Draft comment:
Good pattern for auto-deleting missing triggers. Consider explicitly logging the external_id and resource_path to aid debugging. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. backend/windmill-api/src/native_triggers/nextcloud/external.rs:133
- Draft comment:
When parsing error responses, consider logging or propagating full error details instead of using unwrap_or_default(), to preserve diagnostic info. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. frontend/src/lib/components/triggers.ts:41
- Draft comment:
Duplicate entry for 'email' in TriggerType. Remove the duplicate to avoid confusion. - 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/AddTriggersButton.svelte:110
- Draft comment:
New Nextcloud item added correctly; ensure that the isServiceAvailable check robustly reflects external configuration. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. frontend/src/lib/components/triggers/native/utils.ts:120
- Draft comment:
Using dynamic import for Nextcloud icon is appropriate; ensure fallback for unsupported services is maintained. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:92
- Draft comment:
Good mechanism for handling auto-deletion errors. Verify that error messages from NativeTriggerService.getNativeTrigger are consistent. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
7. frontend/src/lib/components/triggers/utils.ts:90
- Draft comment:
Ensure the triggerIconMap for 'nextcloud' is correct (line 89), and note that mapping is consistent across the app. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. frontend/src/routes/(root)/(logged)/native_triggers/[service_name]/+page.ts:11
- Draft comment:
Good check to throw a 404 if serviceConfig is undefined – ensures unsupported services are caught early. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. frontend/src/lib/components/triggers/native/utils.ts:130
- Draft comment:
Overall utility functions look clean. Consider adding unit tests for getTemplateUrl and isServiceAvailable to cover edge cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
10. frontend/src/lib/components/triggers/AddTriggersButton.svelte:109
- Draft comment:
Ensure that the derived store for addTriggerItems updates when availableNativeServices changes. This appears correctly implemented. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_MxzvpzdQ1QqyAaQB
You can customize 
| if (!externalData) { | ||
| externalDataApplied = false | ||
| } | ||
| }) |
There was a problem hiding this comment.
Typo/consistency note: In this file, the product name is sometimes rendered as 'NextCloud' (e.g. in the error message here: 'Failed to load NextCloud events:') and elsewhere as 'Nextcloud' (e.g. in the UI header). Consider using a consistent capitalization throughout the file.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed e8eebbb in 2 minutes and 5 seconds. Click for details.
- Reviewed
432lines of code in13files - Skipped
0files when reviewing. - Skipped posting
13draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/.sqlx/query-07168aaf14cb6beff0ad4274b441f7f387f5055c47f493271d26731336257384.json:49
- Draft comment:
Changed nullability for the first five columns from true to false. Verify that no null values will be inserted into these columns. - 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 no null values will be inserted into columns that have had their nullability changed. This falls under asking the author to ensure behavior is intended, which is against the rules.
2. backend/.sqlx/query-16e4b1bead9fc77fd98658b8cb8cc6d6bf1df758b30e99bd661da866062ef14f.json:1
- Draft comment:
Entire query file has been removed. Confirm that this obsolete query is no longer needed by the application. - 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.
3. backend/.sqlx/query-1a54356c1e1353950bf6ab1d25ab21270131e9e93ca10d195664e7e5a774fe9e.json:62
- Draft comment:
Removed enum values 'schedule_handler_old' and 'dynamic_skip' from the allowed script kinds, leaving only 'preprocessor'. Ensure this removal doesn’t affect existing triggers. - 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. backend/.sqlx/query-3162ec92bb32af47a71cc41172cc740b5dea1304ce4dfdb4d3d0efa4266f38c5.json:232
- Draft comment:
Added 'nextcloud' to the list of trigger kinds. Verify that this new option is supported in the downstream logic. - 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 the new option is supported in the downstream logic. This falls under the rule of not asking the author to ensure the behavior is intended or to double-check things. Therefore, this comment should be removed.
5. backend/.sqlx/query-804fc11e35f4afc0db194b6fe2594f91df7e588d4d2431bc85f4d8734920c8bf.json:1
- Draft comment:
Removed legacy insert query for 'script'. Ensure that any functionality relying on this query has been updated. - 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.
6. backend/.sqlx/query-85705fc3d7f8ba5f1b12d5fb222c38fc64deb1226aab9dc3bc4465324fce37d1.json:1
- Draft comment:
Removed the insert query for v2_job_queue. Confirm that this query is not invoked in the application anymore. - 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. backend/.sqlx/query-8d67ed8e1271a27b072a8e8ae9973e372949f5ec7d71b80d1ba3eaddf851adce.json:118
- Draft comment:
Added 'nextcloud' to the allowed trigger kinds in the job insertion query. Verify integration with the rest of the trigger processing. - 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 integration with the rest of the trigger processing, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue.
8. backend/.sqlx/query-a0b3e10e077d30c1da135dff9feca3761d400391f1f46a8294da3e6c9af63887.json:1
- Draft comment:
Removed the insert query for v2_job_status. Make sure any status updates now use an alternative query. - 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.
9. backend/.sqlx/query-ab04cda71f8e2be9acbecabe1ee5ef756b8e5c1955fbe111df9ee171dc262338.json:1
- Draft comment:
Removed the job insertion query that returns an ID. Confirm that downstream code does not depend on this query's output. - 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.
10. backend/.sqlx/query-b3dbdfb50ee8118bdaed3164b210cb549a34b96554ae1872355b90304f5dcb76.json:20
- Draft comment:
Swapped the nullable flags: 'email' is now non-nullable while 'username' is nullable. Confirm that this ordering matches the database schema expectations. - Reason this comment was not posted:
Comment looked like it was already resolved.
11. backend/.sqlx/query-b4a9abcb38997587b28655b0f4a212a5bd4039b57fab20b163617e33a4c9dd46.json:1
- Draft comment:
Removed the job runtime insertion query. Ensure this removal does not affect job runtime handling. - 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.
12. backend/.sqlx/query-ff0403790674cdb07022af71c2377afbd8b3a660b3be27514b517c077c63c238.json:1
- Draft comment:
Removed the alternative job insertion query. Confirm that all job creation flows have been updated accordingly. - 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.
13. backend/windmill-git-sync/src/lib.rs:29
- Draft comment:
Removed the NativeTrigger variant from the DeployedObject enum and its associated match arms. Verify that native trigger functionality, if needed, is implemented elsewhere. - 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.
Workflow ID: wflow_RBYavaz7sgLK0Wal
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4418cd9 in 1 minute and 28 seconds. Click for details.
- Reviewed
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/native_triggers/handler.rs:290
- Draft comment:
Removed debug print. Consider using tracing/logging if needed in production. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. backend/windmill-api/src/native_triggers/handler.rs:438
- Draft comment:
Removed debug print. Use proper logging instead of println! for the webhook body if needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_UeZC4x0q6wfsH4om
You can customize
rubenfiszel
reviewed
Oct 11, 2025
|
|
||
| const SYNC_INTERVAL: u64 = 10 * 60; | ||
|
|
||
| pub fn start_sync_loop( |
There was a problem hiding this comment.
the sync loop is reinventing the wheel of what is in monitor.rs, can be fully removed and sync_all_triggers called from there.
rubenfiszel
reviewed
Oct 11, 2025
Comment on lines
+10700
to
+10740
| /native_triggers/{service_name}/w/{workspace_id}/webhook/{internal_id}: | ||
| post: | ||
| summary: receive webhook from external native trigger service | ||
| operationId: nativeTriggerWebhook | ||
| tags: | ||
| - native_trigger | ||
| parameters: | ||
| - name: service_name | ||
| in: path | ||
| required: true | ||
| schema: | ||
| $ref: "#/components/schemas/NativeServiceName" | ||
| - name: workspace_id | ||
| in: path | ||
| required: true | ||
| schema: | ||
| type: string | ||
| - name: internal_id | ||
| in: path | ||
| required: true | ||
| schema: | ||
| type: integer | ||
| format: int64 | ||
| description: The internal database ID of the trigger | ||
| requestBody: | ||
| description: webhook payload from external service | ||
| required: false | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| additionalProperties: true | ||
| text/plain: | ||
| schema: | ||
| type: string | ||
| responses: | ||
| "200": | ||
| description: webhook received successfully | ||
| content: | ||
| text/plain: | ||
| schema: |
There was a problem hiding this comment.
there is no need to create a new webhook endpoint. The default generated webhook endpoint + query arg for service name + internal id is sufficient
rubenfiszel
reviewed
Oct 11, 2025
| &authed, | ||
| Some(user_db.clone()), | ||
| &db, | ||
| &data.resource_path, |
There was a problem hiding this comment.
no, it comes from a workspace setting
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed f45142a in 3 minutes and 41 seconds. Click for details.
- Reviewed
4891lines of code in33files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/common/index.ts:20
- Draft comment:
The export for the Url component now imports from './Text.svelte'. Please double‐check that this renaming is intentional and that all references are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to double-check their work, which violates the rule against asking for confirmation or double-checking. It does not provide a specific suggestion or ask for a test to be written. Therefore, this comment should be removed.
2. frontend/src/lib/components/triggers/native/utils.ts:128
- Draft comment:
The getServiceIcon function only handles the 'nextcloud' case without a default branch. For services not explicitly handled, consider providing a default icon or explicit undefined handling to make the behavior clear. - 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.
3. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:170
- Draft comment:
A debug log prints the payload before sending it. Ensure that such verbose logging (especially with potentially sensitive data) is removed or gated under a debug flag in production. - Reason this comment was not posted:
Comment was on unchanged code.
4. backend/windmill-api/openapi.yaml:10487
- Draft comment:
Typographical/grammatical note: In the summary for the '/w/{workspace}/native_triggers/integrations/{service_name}/exists' endpoint, the phrase "check if integrations for a particular service exists" has a subject-verb agreement issue. Consider revising it to "check if integrations for a particular service exist". - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a typographical/grammatical issue in a summary. It does not provide a code suggestion or address a potential issue in the code logic. According to the rules, purely informative comments should be removed.
5. backend/windmill-api/openapi.yaml:19854
- Draft comment:
Typographical note: In the RedirectUri schema, thetypefor theredirect_uriproperty is indented on a separate line. To improve readability and avoid potential YAML interpretation errors, consider changing it from: type: string to: type: string - 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 comment is technically correct about the inconsistent formatting, both formats are valid YAML. The current format will work fine and is just a matter of style. The comment doesn't point out a bug or potential issue, just a stylistic preference. Our rules state we should not make purely informative comments or comments about obvious things. The inconsistent formatting could potentially make the file harder to maintain or lead to confusion for other developers. Maybe there's a team style guide that prefers one format over the other? Even if there is a style guide, this is a minor formatting issue that doesn't affect functionality. OpenAPI/YAML parsers will handle both formats equally well. Delete the comment as it's purely about formatting style and doesn't highlight any actual issues that need to be fixed.
6. backend/windmill-api/src/native_triggers/nextcloud/external.rs:197
- Draft comment:
Typographical issue: the error message reads "Nextcloud native trigger only support webhook event". Consider changing this to "NextCloud native trigger only supports webhook events" for grammatical correctness. - 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% While the grammar correction is technically accurate, error messages are internal implementation details that don't significantly impact functionality. The current message is still clear and understandable despite the minor grammatical issues. This kind of nitpicky comment about grammar in error messages doesn't meet the bar for "clearly a code change required" per the review rules. The error message is indeed grammatically incorrect, and fixing it would make the code more professional. Poor grammar in user-facing messages could reflect badly on the project. While professional polish is good, this is an internal error message that would only be seen by developers in rare error cases. The benefit doesn't justify the overhead of a code change and review. Delete this comment as it's too minor and doesn't meet the bar for requiring a code change.
7. backend/windmill-api/src/workspaces.rs:2087
- Draft comment:
There appears to be a naming inconsistency: the variable has been renamed totriggers_usedbut the SQL query alias remains ` - 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.
8. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:285
- Draft comment:
Typo: 'Re-Generate' should likely be 'Regenerate' for consistency with the first message. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a very minor UI text change. While technically correct that "Regenerate" is more standard than "Re-Generate", this kind of nitpicky comment about UI text doesn't meet our bar for comments. The rules state not to comment on pure UI changes and to only comment if there's clearly a code change required. This is purely cosmetic and doesn't affect functionality. The hyphenated version "Re-Generate" could potentially be argued to be more readable by visually separating the prefix. Maybe there's a house style guide that prefers hyphenated versions? Even if there was a style guide preference, this kind of minor UI text formatting doesn't warrant a PR comment. The functionality is clear either way. Delete this comment as it's too minor and relates purely to UI text formatting rather than functionality.
9. frontend/src/lib/components/triggers/native/services/nextcloud/NextcloudTriggerForm.svelte:198
- Draft comment:
It looks like the closing tag is split over two lines (line 198 and 199). This might be an unintended typographical error. Consider placing the '>' on the same line as the closing tag (i.e.,</div>) to improve readability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a purely stylistic suggestion about HTML formatting in a template. It doesn't affect functionality. The current format actually looks intentional and consistent with Svelte's formatting style. The split tag allows the content to be aligned with its opening tag, which can be considered good formatting. Maybe there are established formatting guidelines for this project that prefer single-line closing tags? Maybe the split tag does make the code harder to read for some developers? Even if there are formatting preferences, this is too minor of an issue to warrant a comment. The current format is perfectly readable and common in template files. This comment should be deleted as it's purely about formatting style, doesn't affect functionality, and the current format is perfectly acceptable.
10. frontend/src/lib/components/triggers/native/services/nextcloud/NextcloudTriggerForm.svelte:226
- Draft comment:
Typographical note: The term "NextCloud" is used here. If your project naming convention prefers "Nextcloud" (as appears in file paths and elsewhere), please consider updating the casing 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 consistency in product naming is good, this seems like a minor stylistic issue. The comment is about UI text, which falls under the "Do NOT comment on pure frontend UI or styling changes" rule. Additionally, since there's inconsistency throughout the file, this single-line change wouldn't fully solve the problem. The inconsistency could cause confusion for users if it appears differently in different places. Also, there might be a company style guide that dictates the correct casing. Even if there is a style guide, this kind of UI text consistency issue should be handled through a separate cleanup effort, not as part of code review comments on individual lines. Delete the comment. It violates the rule about not commenting on UI/styling changes, and a single-line casing fix wouldn't solve the broader consistency issue.
Workflow ID: wflow_YeIkR0GOIGLeSRAV
You can customize
| let customRawConfig = $state('') | ||
| let token = $state('') | ||
| let request_type: 'async' | 'sync' = $state('async') | ||
| let event_type: 'webhook' = $state('webhook') |
There was a problem hiding this comment.
Avoid shadowing the 'event_type' state variable. A state variable named 'event_type' is defined (line 57) and then re‐declared as an object inside the save() function (line ~160). Consider renaming the locally constructed event object (e.g., to payloadEvent) to prevent confusion.
| size="xs" | ||
| color="light" | ||
| variant="border" | ||
| onclick={() => checkServiceAvailability()} |
There was a problem hiding this comment.
Typo: The attribute 'onclick' should be 'on:click' to correctly bind the event in Svelte.
Suggested change
| onclick={() => checkServiceAvailability()} | |
| on:click={() => checkServiceAvailability()} |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 9ac416a in 2 minutes and 38 seconds. Click for details.
- Reviewed
2073lines of code in59files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/scopes.rs:45
- Draft comment:
Thefrom_scope_stringfunction splits the scope string into 2, 3, or 4 parts. The handling of 3-part scopes (line 60–70) relies on the domain being 'jobs:run'. Consider adding more explicit documentation or error handling to distinguish when a 3-part scope should be interpreted as having a 'kind' versus a resource list. - 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.
2. backend/windmill-api/src/users.rs:2657
- Draft comment:
The helper functionupdate_username_in_workpsacecontains multiple SQL UPDATE calls that use.unwrap()(e.g. at line 2810). Using unwrap here can cause runtime panics on error. It's recommended to propagate errors using the?operator for more robust error handling. - 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.
3. backend/windmill-api/src/users.rs:1387
- Draft comment:
In theconvert_user_to_groupfunction, JSON parsing for group roles usesunwrap_or_default(). While this prevents crashes, it can silently mask JSON parsing errors. Consider explicitly handling these errors to alert developers in case of malformed data. - 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/native/NativeTriggerEditor.svelte:157
- Draft comment:
In thesave()function, the event payload is constructed with a hard-coded event type{ request_type, type: 'webhook' }. While this aligns with current requirements, consider adding additional field validations (beyond checking onlyrunnable_path) to ensure the payload is complete and correct. - 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 code changes show removal of token-related functionality and simplification of the event_type handling. The existing validation appears sufficient for the simplified payload. The comment is speculative and doesn't point to any specific issues. It's suggesting additional validation without identifying actual problems. I could be wrong about the validation being sufficient - there might be business rules or requirements I'm not aware of. The comment might be hinting at known issues in production. However, without specific evidence of validation problems, this comment is speculative and asks for changes without clear justification. The code already has validation mechanisms in place. The comment should be deleted as it makes a speculative suggestion without identifying concrete issues, and the code already has appropriate validation.
5. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:135
- Draft comment:
The derived state forvalidationErrorsonly validatesrunnable_path. It may be beneficial to expand validations to cover additional fields required for a trigger, ensuring comprehensive client‐side validation. - 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.
6. frontend/src/lib/components/triggers/native/utils.ts:59
- Draft comment:
The functiongetAvailableNativeTriggerServicesiterates over available services with a sequentialawaitin a loop. For better performance, consider usingPromise.allto run these service availability checks in parallel. - 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.
Workflow ID: wflow_CWo4OHFKIYv2kfC2
You can customize
| let _ = handler.validate_data_config(&data); | ||
| let mut tx = user_db.begin(&authed).await?; | ||
|
|
||
| let EventType::Webhook(webhook) = &mut data.event_type; |
There was a problem hiding this comment.
When pattern matching on data.event_type as a Webhook (line 94), the code assumes that the variant is always Webhook. If a different variant is ever passed, this will panic. Consider using a safe pattern with an if let or proper error return on mismatched event type.
|
|
||
| let existing = get_native_trigger(&mut *tx, &workspace_id, id, service_name).await?; | ||
|
|
||
| let EventType::Webhook(exist_webhook_token) = existing.event_type.0; |
There was a problem hiding this comment.
Similarly, in the update trigger handler, the code directly destructures the existing trigger’s event type (line 181) assuming it is Webhook. Consider guarding against unexpected variants to prevent runtime panics.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 87ffc7d in 2 minutes and 9 seconds. Click for details.
- Reviewed
126lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/Cargo.lock:4519
- Draft comment:
Removed explicit version for derive_builder_macro and removal of duplicate derive_builder package. Ensure that relying on unpinned dependencies does not introduce version conflicts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is about dependency changes, specifically the removal of an explicit version and a duplicate package. It advises ensuring that unpinned dependencies do not introduce version conflicts. This falls under the rule of not commenting on dependency changes or library versions that are not recognized. The comment is not specific enough to be useful and violates the rule against asking the author to ensure something without a specific suggestion.
2. backend/Cargo.lock:10228
- Draft comment:
quick-xml version downgraded from 0.31.0 to 0.30.0. Confirm that this downgrade is intentional and does not break functionality. - 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.
3. backend/Cargo.lock:11408
- Draft comment:
Samael dependency now switches from a Git source to the registry and updates base64 from 0.22.1 to 0.21.7. Verify that these changes (including any breaking changes in samael) are compatible with the codebase. - 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. backend/Cargo.toml:343
- Draft comment:
Duplicate samael dependencies found: one using a Git source and one using a version from crates.io. Remove one entry (preferably the Git one, since Cargo.lock uses the registry version) to avoid conflicts. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_M1SMlSa7FkOjbK3P
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 378e4cd in 3 minutes and 27 seconds. Click for details.
- Reviewed
391lines of code in8files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/native_triggers/mod.rs:36
- Draft comment:
The TryFrom for ServiceName implementation is clear and uses strum effectively. Consider adding a doc comment to explain the expected input format and the error message (which dynamically lists supported services). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The implementation is quite straightforward - it converts strings to enum variants and provides a descriptive error message. The input format is obvious (lowercase service names) and the error message is self-explanatory. A doc comment would just repeat what's already clear from the code. The comment is purely informative and doesn't suggest any actual code changes needed. The comment does point out good practices around documentation. Clear API documentation can be valuable for other developers. While documentation is valuable, this case is simple enough that the code is self-documenting. The error message already provides the key information a user would need. Delete the comment as it only makes an optional documentation suggestion for a straightforward implementation that is already clear from the code itself.
2. backend/windmill-api/src/native_triggers/mod.rs:133
- Draft comment:
The External trait is well‐structured with clear associated types and method signatures. Consider adding documentation comments for each method to clarify their usage and expected behavior for future maintainers. - 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.
3. backend/windmill-api/src/native_triggers/nextcloud/external.rs:27
- Draft comment:
The AuthMethod enum defines both 'Null' and 'None' variants. Their names are very similar and may cause confusion. Consider renaming one of them (e.g. use 'NoAuth' instead of 'None') for clarity. - 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/native/utils.ts:24
- Draft comment:
The template URLs for the Nextcloud service are updated (script template uses 'hub/19826/...' while flow uses 'hub/19824/...'). Please double-check that this intentional discrepancy is correct. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. backend/windmill-api/src/jobs.rs:4291
- Draft comment:
Typographical error: 'Paath' should be 'Path'. - 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% Debug print statements are not critical to functionality and are only used during development. The typo doesn't affect functionality. Additionally, debug prints are often temporary and may be removed later. The comment is pointing out something very minor that doesn't impact code quality or behavior. Could the typo make the debug output slightly harder to search for? Could it confuse other developers? The impact would be minimal since this is just a debug print. The message is still understandable and searchable. Debug prints are not part of the public API or user-facing functionality. This comment should be deleted as it addresses a minor typo in a debug print statement that doesn't affect functionality or code quality.
Workflow ID: wflow_FzpEcH2kbaybmg3O
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2d447ff in 2 minutes and 19 seconds. Click for details.
- Reviewed
1465lines of code in59files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/jobs.rs:4311
- Draft comment:
Avoid using debug print (println!) in production. Replace with a logging macro (e.g. tracing::debug!) so debug output can be controlled. - 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.
2. backend/windmill-api/src/jobs.rs:6960
- Draft comment:
Consider replacing the hard‐coded polling sleep intervals in the SSE update loop with configurable values or Tokio’s interval API to improve readability and avoid tightly coupled magic numbers. - 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.
3. backend/windmill-api/src/jobs.rs:4020
- Draft comment:
Ensure that permission checks (e.g., using check_scopes) provide clear error messages and are consistently applied in complex functions like run_flow_by_path_inner. Adding inline comments or doc comments here can clarify the expected behavior. - 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. backend/windmill-api/src/jobs.rs:7240
- Draft comment:
Review the embedded SQL queries for performance and maintainability. Consider extracting particularly complex queries into separate helper functions or constants. - 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. backend/windmill-api/src/jobs.rs:7800
- Draft comment:
Consider handling Option values explicitly rather than using unwrap_or_default() to avoid silently masking unexpected missing data. - 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.
6. backend/.sqlx/query-280a361076d1c6317610765960f543252891c53351bdc98da66cc30ffc895866.json:3
- Draft comment:
Typographical note: In the query string on this line, there appears to be a double space between 'v2_as_completed_job' and 'WHERE'. If this is unintentional, please consider correcting it. - 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% Extra spaces in SQL queries don't affect functionality. This is purely a style/formatting issue. The comment is technically correct about the double space, but it's an extremely minor issue that doesn't impact code quality or behavior. We should avoid nitpicking about such trivial formatting issues. The extra space could be considered a consistency issue if the codebase has strict formatting standards. Some teams do care about exact SQL formatting. Even if there are formatting standards, an extra space in a SQL query is too minor to warrant a PR comment. SQL is generally whitespace-insensitive, and this doesn't affect readability significantly. Delete this comment as it's too trivial and doesn't meaningfully improve code quality. It violates the rule about not making obvious or unimportant comments.
7. backend/.sqlx/query-6c0f74c56789ac51ccb06cd8a14986071ccc94df0de137b56d63d673db11d8aa.json:7
- Draft comment:
The key "Left" starts with a capital letter. If this is intended, it's fine; however, if it should follow typical lower-case conventions (e.g., "left"), consider making this change. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% 1. This is an auto-generated file by sqlx, not hand-written code. 2. "Left" is likely part of sqlx's internal schema/format. 3. Changing auto-generated code is generally not recommended. 4. The capitalization is probably intentional and required by the sqlx library. I could be wrong about this being auto-generated, or there could be a configuration option in sqlx to change this format. Even if there are configuration options, suggesting style changes to auto-generated files is not useful and could cause issues with the tooling. Delete this comment as it suggests modifying an auto-generated file's format, which could break the tooling and provides no value.
8. backend/.sqlx/query-7c9a464ac807051b99fe37f2078f1b17f824e6d9b1124db618855a15a98e31f6.json:7
- Draft comment:
Typographical note: The key "Left" appears to be capitalized, while other keys in the JSON are lowercase. If this is unintentional, consider renaming it to "left" 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 = 0% vs. threshold = 50% This appears to be an auto-generated file by SQLx (a Rust SQL toolkit). The capitalization of "Left" is likely intentional and part of SQLx's schema. Making changes to auto-generated files is generally not recommended. Even if it wasn't auto-generated, this would be a very minor stylistic issue that doesn't impact functionality. I could be wrong about this being auto-generated. Maybe it's hand-written and the capitalization really is a mistake. The .sqlx directory and file naming pattern strongly suggests this is auto-generated. Even if it's not, the comment is too minor to be worth keeping. Delete this comment as it's suggesting changes to what appears to be an auto-generated file, and even if it wasn't, the issue is too minor to warrant a comment.
9. backend/.sqlx/query-9da0cea2a5d0464ca78cfeccf6cedf2b1c0e6e6cb3c9183a937a68465debdb06.json:3
- Draft comment:
There appears to be an extra space between 'WHERE' and 'ended_at' in the SQL query. Consider removing the additional space unless it is intentional. - 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% Extra spaces in SQL queries don't affect functionality - SQL is whitespace-insensitive. The comment is purely stylistic and doesn't impact the logic or behavior of the code. While consistent formatting is nice, this is an extremely minor issue that doesn't warrant a PR comment. Additionally, this appears to be in an auto-generated or metadata file. Perhaps consistent formatting is more important than I think? Maybe there are SQL parsers or tools that care about exact whitespace? SQL is definitively whitespace-insensitive, and this is a minor stylistic issue at best. Even if there were tools that cared about whitespace, they would normalize it themselves. Delete this comment as it points out an extremely minor stylistic issue that has no impact on functionality.
10. backend/.sqlx/query-a00e61e770e20157bbd9e4cdedf7fb5f9de7c8c9e50282e3ecf2e3ce917ec37a.json:7
- Draft comment:
Typo(?) detected: The key 'Left' might be a mistake; did you mean to use 'left' (or another name) to maintain consistent capitalization? - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is clearly a generated file, not hand-written code. The "Left" key is likely part of the sqlx tool's schema and is intentionally capitalized. Making changes to generated files is generally not a good practice. The capitalization is probably correct as-is since it's part of the tool's output. I could be wrong about this being a generated file, or there could be a real issue with the sqlx tool's output that should be reported upstream. The .sqlx directory and file format strongly suggests this is generated code, and the hash value confirms it. We should not modify tool-generated files unless there's a problem with the tool itself. Delete this comment as it's suggesting a change to a generated file where the capitalization is likely intentional and correct.
Workflow ID: wflow_0fdShC9WT3J9NXwR
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed cd0613b in 2 minutes and 2 seconds. Click for details.
- Reviewed
94lines of code in4files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/users.rs:2158
- Draft comment:
Error message mismatch: In the impersonate function, the check is on impersonate_email but the error message reads 'impersonate_username is required'. Please update the error message for consistency. - 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.
2. backend/windmill-api/src/users.rs:2607
- Draft comment:
Typo in error message: 'Usermame' should be 'Username'. - 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.
3. backend/windmill-api/src/users.rs:2658
- Draft comment:
Function naming typo: 'update_username_in_workpsace' appears to have a typo. Consider renaming it to 'update_username_in_workspace'. - 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. backend/windmill-api/src/users.rs:3048
- Draft comment:
Avoid using unwrap() on async query results in update_username_in_workpsace. Use the '?' operator to propagate errors instead of panicking. - 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. backend/windmill-api/src/users.rs:2658
- Draft comment:
Consider refactoring the extensive update_username_in_workpsace function into smaller, modular helper functions to improve readability and maintainability. - 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.
Workflow ID: wflow_EFWCUVKEFyWqMXoo
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1e723df in 1 minute and 12 seconds. Click for details.
- Reviewed
54lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:29
- Draft comment:
Refactored to use getTemplatePath. Verify that the service config now provides a complete URL/path since the URL is no longer constructed here. - 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 the service config provides a complete URL/path, which is against the rules as it asks for confirmation of intention. It doesn't provide a specific code suggestion or ask for a test to be written.
2. frontend/src/lib/components/triggers/native/utils.ts:25
- Draft comment:
Templates revised to store full paths (e.g. '/scripts/add?hub=hub%2F19826'). Confirm that this format with pre-encoded values is intended for all use cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding the use of pre-encoded values in paths. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.
3. frontend/src/lib/components/triggers/native/utils.ts:135
- Draft comment:
Renamed getTemplateUrl to getTemplatePath. Consider aligning variable names (e.g. scriptTemplateUrl vs. scriptTemplatePath) in the component for clarity. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_LwB66OFcC8DmEz7U
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 75772c4 in 2 minutes and 10 seconds. Click for details.
- Reviewed
229lines of code in5files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/icons/NextcloudIcon.svelte:8
- Draft comment:
Default size changed from '24px' to 16. Verify that this new numeric value (assumed to be pixels) is intentional and consistent with other icons. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:288
- Draft comment:
There is a commented-out block of Tabs code. Consider removing it if it's not needed to reduce clutter. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
3. frontend/src/lib/components/triggers/native/NativeTriggerTable.svelte:25
- Draft comment:
Share functionality (Share icon and shareModal prop) has been removed. Confirm that this removal is intentional. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte:600
- Draft comment:
Tab value updated from 'integrations' to 'native_triggers'. Ensure that downstream logic and dynamic import for WorkspaceIntegrations remain consistent. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:258
- Draft comment:
It appears that the HTML comment starting here is not closed with a '-->'. Please add the closing tag to properly terminate the comment. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:288
- Draft comment:
There appears to be a stray "-->" on this line. It seems like an incomplete HTML comment marker. If this is not intended, please remove or complete it. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_j90EsUGYUliUbwKu
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 1fe5b6f in 2 minutes and 3 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_2zAayFq7ntRvlVql
You can customize
| let clientSecret = $state('') | ||
| let baseUrl = $state('') | ||
| let workspace = $workspaceStore! | ||
| redirectUri = `${window.location.origin}/workspace_settings?tab=native_triggers&service=${serviceName}&workspace=${workspace}` |
There was a problem hiding this comment.
The redirect URI now uses tab=native_triggers instead of integrations. Ensure that any fallback or UI display (e.g., in the Alert) is updated for consistency.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ae3a1b8 in 1 minute and 50 seconds. Click for details.
- Reviewed
45lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/triggers/native/NativeTriggerEditor.svelte:332
- Draft comment:
Updated button link and label to use the native_triggers tab. Ensure that the new tab identifier exists in workspace settings for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. frontend/src/routes/(root)/(logged)/native_triggers/[service_name]/+page.svelte:44
- Draft comment:
Added toast notification when serviceAvailable is false. Confirm that repeated invocations of checkServiceAvailability don’t overwhelm the user with multiple toasts. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. frontend/src/routes/(root)/(logged)/native_triggers/[service_name]/+page.svelte:179
- Draft comment:
Updated the 'Manage' button's href and label to point to the native_triggers tab. Verify that this URL is consistent with the rest of the application. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_bnvtMPdRYPV51EKo
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
🌀 Nextcloud Integration for Windmill
This pull request introduces a native integration between Windmill and Nextcloud, enabling users to seamlessly connect their Windmill workspace with their Nextcloud instance via OAuth2.
Once connected, users can access Nextcloud triggers to monitor specific events such as file uploads, updates, or deletions and automatically run scripts or complex workflows in response.
🎥 Demo
The demo video below illustrates the complete workflow: connecting Windmill to Nextcloud, syncing the workspace, and setting up a trigger that will responds to the desired Nextcloud event.
windmill-nextcloud-native-trigger-demo.mov