Skip to content

feat: generate http route triggers from openapi spec #5857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Jun 5, 2025

Conversation

dieriba
Copy link
Contributor

@dieriba dieriba commented Jun 3, 2025

Important

Add functionality to generate HTTP route triggers from OpenAPI specifications, including backend endpoints and frontend UI components.

  • Backend:
    • Add /w/{workspace}/http_triggers/create_many endpoint in openapi.yaml for creating multiple HTTP triggers.
    • Refactor http_triggers.rs to support batch creation with create_many_http_trigger().
    • Introduce HttpMethod enum in http_trigger_args.rs for HTTP methods.
  • Frontend:
    • Add HttpTriggersGenerator.svelte for generating HTTP routes from OpenAPI specs.
    • Update package.json to include @apidevtools/swagger-parser and openapi-types.
    • Modify vite.config.js to use vite-plugin-node-polyfills.
    • Add FolderPicker.svelte for folder selection in route generation.
  • Utilities:
    • Implement OpenAPI parsing in utils.ts with OpenApi namespace.
    • Add generateHttpTriggerFromOpenApi() in utils.ts for processing OpenAPI documents.

This description was created by Ellipsis for 6cd6d02. You can customize this summary. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented Jun 3, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2a598c8
Status: ✅  Deploy successful!
Preview URL: https://7b81118b.windmill.pages.dev
Branch Preview URL: https://dieri-generate-http-route-tr.windmill.pages.dev

View logs

@dieriba dieriba marked this pull request as ready for review June 3, 2025 16:12
@dieriba dieriba marked this pull request as draft June 3, 2025 16:13
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to 1e3fefe in 4 minutes and 9 seconds. Click for details.
  • Reviewed 1089 lines of code in 12 files
  • Skipped 1 files when reviewing.
  • Skipped posting 11 draft 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/http/utils.ts:86
  • Draft comment:
    The conversion function strips the leading '/' by slicing; ensure that behavior is intended and handles edge cases (e.g. paths not starting with '/').
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that the behavior is intended and handles edge cases, which violates the rule against asking for confirmation of intention or ensuring behavior. It does not provide a specific suggestion or ask for a specific test to be written.
2. frontend/src/lib/utils.ts:449
  • Draft comment:
    The removeKeysWithEmptyValues function currently only deletes keys with undefined values. Consider if null or empty strings should also be removed, and note that it mutates the input object.
  • 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/routes/(root)/(logged)/routes/+page.svelte:317
  • Draft comment:
    Ensure that building URLs via getHttpRoute and copying them to clipboard does not introduce injection risks if any user-provided path is unsanitized.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
4. frontend/vite.config.js:70
  • Draft comment:
    A custom alias is defined for the 'monaco-editor' hover module; verify that this alias is still compatible with the latest library versions to avoid breakage.
  • Reason this comment was not posted:
    Confidence changes required: 40% <= threshold 50% None
5. backend/.vscode/settings.json:13
  • Draft comment:
    Typographical error: 'restructring triggers, decoding trigger message on work' should likely be corrected to 'restructuring triggers, decoding trigger message on work' or similar.
  • Reason this comment was not posted:
    Comment was on unchanged code.
6. frontend/src/lib/components/HttpRouteGenerator.svelte:97
  • Draft comment:
    Typo: The error message reads "Please enter an openapi spec first". For consistency, consider capitalizing "OpenAPI" (i.e. "Please enter an OpenAPI spec first").
  • 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% While the comment is technically correct about inconsistent capitalization, I need to consider if this is important enough to keep. The rules say not to make comments that are obvious or unimportant. This is a very minor UI text change that doesn't affect functionality. The rules also say to ignore pure frontend UI changes. The inconsistency could be seen as a legitimate issue for maintaining professional polish in user-facing messages. Also, "OpenAPI" is a proper noun/trademark that should be capitalized correctly. While proper capitalization is good, this is ultimately just a UI text change. The rules explicitly state not to comment on UI changes and to assume the author made UI choices correctly. This comment should be deleted as it violates the rule about not commenting on pure UI/frontend changes, even though the suggestion itself is technically correct.
7. frontend/src/lib/components/HttpRouteGenerator.svelte:152
  • Draft comment:
    Typographical inconsistency in the tooltip text: "Enter an OpenApi URL to generate an HTTP routes from it". Suggested revision: "Enter an OpenAPI URL to generate HTTP trigger(s) from it" (note the consistent use of "OpenAPI" and matching terminology with other parts of the UI).
  • 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% While this is a new file, UI text consistency issues are generally not critical enough to warrant a PR comment. The rules state "Do NOT comment on any pure frontend UI or styling changes". Even though this is about consistency, it's still fundamentally a UI text change. The functionality is not impacted by this capitalization or wording difference. The inconsistent terminology could potentially confuse users if they're trying to match terms across the UI. The comment is technically correct about the inconsistency. While technically correct, this kind of minor UI text consistency issue should be handled through a style guide or UI review process, not as PR comments. It doesn't impact functionality. Delete this comment as it violates the rule about not commenting on pure UI changes.
8. frontend/src/lib/components/HttpRouteGenerator.svelte:191
  • Draft comment:
    Consider revising the placeholder text "OpenApi URL" to "OpenAPI URL" for consistency with other references.
  • 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% While consistency in UI text is good, this is a very minor UI text change. The comment is about pure UI text, not logic. According to the rules, we should "NOT comment on any pure frontend UI or styling changes" and should "assume that if the author changed the UI, they did it correctly". This falls into that category. The inconsistency in capitalization could be confusing to users and maintaining consistent branding is important for professional software. While consistency is good, the rules explicitly state not to comment on pure UI changes. This is exactly the kind of minor UI detail that the rules say to ignore. Delete this comment as it violates the rule about not commenting on pure UI/styling changes.
9. frontend/src/lib/components/triggers/http/utils.ts:3
  • Draft comment:
    Typo: The module name 'random_positive_adjetive' appears to be misspelled. It should likely be 'random_positive_adjective'.
  • 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 real typo in a newly added import path. Typos in import paths are actually important because they could cause runtime errors if the file name doesn't match. However, if this was a problem, it would be caught immediately by the build system when it tries to resolve the import. The build system would fail with a "module not found" error. The typo could be intentional - maybe the file is actually named that way in the codebase. Without seeing the actual filesystem, we can't be 100% sure this is wrong. While the filename could theoretically be intentionally misspelled, that would be very unusual. However, the more important point is that if the import path was wrong, the build would fail immediately. The comment should be deleted because import path issues would be caught by the build system - we don't need manual review comments for this.
10. frontend/src/routes/(root)/(logged)/routes/+page.svelte:194
  • Draft comment:
    Typographical suggestion: Consider changing "Generate http trigger(s) from OpenApi spec" to "Generate HTTP triggers from OpenAPI spec" for consistency with common nomenclature.
  • 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% While the suggestion is technically correct regarding standard capitalization of HTTP and OpenAPI, this is a UI text change that doesn't affect functionality. The rules specifically state not to comment on pure frontend UI or styling changes, assuming the author made UI choices deliberately. The comment doesn't point out a logic issue or code quality problem. The capitalization inconsistency could potentially cause confusion for users or developers. The standard capitalization exists for a reason - these are proper names/acronyms. Despite the valid point about standard capitalization, this is still fundamentally a UI text change. The rules are clear about not commenting on UI/styling changes, as these are considered intentional choices by the author. Delete this comment as it violates the rule about not commenting on pure frontend UI and styling changes.
11. frontend/src/routes/(root)/(logged)/routes/+page.svelte:231
  • Draft comment:
    Typographical suggestion: Consider capitalizing 'OpenApi' to 'OpenAPI' for consistency with standard terminology. Also, if appropriate, consider using 'HTTP' instead of 'http' to match typical acronyms.
  • 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 rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic. Assume that if the author changed the UI, they did it correctly." This is a pure UI text change about capitalization. While the suggestion might be technically correct about standard capitalization, it's not a logic issue. The capitalization suggestion is technically correct according to industry standards. The inconsistency could be confusing to users. Despite being technically correct, the rules are very clear that UI changes should be trusted to the author. This is purely a UI text change with no logic implications. Delete the comment as it violates the rule about not commenting on pure UI changes.

Workflow ID: wflow_aVOFEPN7AURSNh1m

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

openApiUrlEditorValue = data
code = data
} catch (error) {
sendUserToast(error.body, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the catch block of fetchOpenApiConfig, error is assumed to have a body property. Consider checking its structure or using a fallback (e.g. error.message) to avoid undefined values.

Suggested change
sendUserToast(error.body, true)
sendUserToast(error.body || error.message, true)

@dieriba dieriba force-pushed the dieri/generate-http-route-triggers-from-openapi-spec branch from 1e3fefe to a95b780 Compare June 4, 2025 08:06
@dieriba dieriba marked this pull request as ready for review June 4, 2025 15:30
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to 8c4dcc3 in 5 minutes and 1 seconds. Click for details.
  • Reviewed 1222 lines of code in 11 files
  • Skipped 1 files when reviewing.
  • Skipped posting 10 draft 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/utils.ts:696
  • Draft comment:
    Extra closing brace in pluralize customPlural branch; likely a typo.
  • 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. frontend/src/lib/components/triggers/http/utils.ts:3
  • Draft comment:
    Consider renaming 'random_positive_adjetive' to 'random_positive_adjective' for clarity.
  • 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% Since this is about a file path, fixing this would require renaming the actual file, not just the import. File renames are explicitly mentioned in the rules as something that would be caught by the build system. The build would fail if the file path was incorrect. Additionally, this seems more like a cosmetic issue than a logic problem. The spelling error could propagate to other parts of the codebase and cause confusion for developers. It's a clear mistake that should be fixed. While fixing typos is good practice, the rules explicitly state not to comment on file renames or things that would be caught by the build. If the import path was wrong, the code wouldn't compile. Delete the comment since it relates to a file rename that would be caught by the build system if incorrect.
3. frontend/src/routes/(root)/(logged)/routes/+page.svelte:73
  • Draft comment:
    Verify that 'routes' is the intended trigger kind. Elsewhere HTTP triggers use 'http'; consider using a consistent trigger kind.
  • 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/http/HttpTriggersGenerator.svelte:218
  • Draft comment:
    Using a manual toggle (forceRerender) to trigger re-rendering is hacky; consider leveraging Svelte’s reactive key blocks or reactive assignments for reactivity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The forceRerender toggle is used to force the SimpleEditor to re-render when the code content changes. While using a manual toggle isn't ideal, in this case it may be necessary because: 1. The SimpleEditor may be a third-party component that doesn't properly handle content updates 2. The code changes are coming from file uploads and API calls where normal reactivity might not trigger properly 3. The #key block usage shows they are following Svelte patterns, just with a manual trigger I could be wrong about the SimpleEditor component - maybe it would handle updates fine without forcing a re-render. The code could potentially be restructured to use Svelte stores or other reactive patterns. While cleaner patterns may exist, the current approach works reliably for handling external content updates. Without more context about the SimpleEditor component's behavior, we can't be certain a better solution exists. While the comment raises a valid point about code quality, the current implementation may be necessary given the constraints. Without more evidence that a better solution exists, this comment risks suggesting changes that could break functionality.
5. frontend/vite.config.js:18
  • Draft comment:
    Review proxy configuration: ensure that target URLs from environment variables are secure and appropriate for production deployments.
  • 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/windmill-api/openapi.yaml:8463
  • Draft comment:
    Typographical consistency: In the requestBody description, consider capitalizing ‘http’ to ‘HTTP’ (e.g., “new HTTP trigger”) for consistency with common terminology.
  • 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. The comment is about a very minor capitalization issue 2. While technically correct that "HTTP" is conventionally capitalized, this is an extremely minor issue 3. The comment doesn't affect functionality or clarity 4. The comment is about a description field that is only visible in API docs 5. The rules state not to make purely informative comments or comments that are obvious/unimportant The comment is technically correct about standard capitalization of "HTTP", but is this really important enough to warrant a PR comment? No, this is exactly the kind of minor, purely informative comment that the rules say to avoid. It doesn't affect functionality and is too trivial to be worth a PR comment. Delete this comment. While technically correct about HTTP capitalization, it's too minor and purely informative to warrant a PR comment per the rules.
7. backend/windmill-api/openapi.yaml:8473
  • Draft comment:
    Typographical consistency: In the response description, consider capitalizing ‘http’ to ‘HTTP’ and potentially using a plural form if appropriate (e.g., “HTTP triggers created”) to align with the operation’s semantics.
  • 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. The comment is about pure text formatting/style, not functionality. 2. The change would make the text more consistent with standard capitalization of "HTTP". 3. Making it plural is more accurate since this endpoint creates multiple triggers. 4. However, this is a very minor cosmetic change that doesn't impact API functionality or understanding. 5. The rules state not to make purely informative comments or comments about obvious/unimportant things. While the suggested changes would technically improve consistency and accuracy, they are extremely minor stylistic changes that don't materially impact the API's usability or documentation clarity. Even though the changes would be technically correct, they fall into the category of "obvious or unimportant" changes that the rules explicitly say to avoid commenting on. Delete this comment as it suggests purely cosmetic text changes that don't meaningfully improve the API documentation.
8. backend/windmill-api/openapi.yaml:8633
  • Draft comment:
    Potential typographical error: The property name 'workspaced_route' may be intended to be 'workspace_route'. Please verify if the name is correct.
  • Reason this comment was not posted:
    Comment was on unchanged code.
9. backend/windmill-api/src/http_triggers.rs:339
  • Draft comment:
    Typo/wording suggestion: In the error message, consider pluralizing "static asset" to "static assets" (and possibly "Static website" to "Static websites") for consistency, i.e., "Static websites and static assets are not supported on cloud."
  • Reason this comment was not posted:
    Comment was on unchanged code.
10. frontend/src/lib/components/triggers/http/utils.ts:3
  • Draft comment:
    Typo: In the import path '$lib/components/random_positive_adjetive', "adjetive" should be spelled as "adjective".
  • 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 a real typo in a newly added import statement. Incorrect import paths will cause build failures, but this specific case might not since it's about the actual filename being misspelled. If the file exists with the misspelled name, it would work but propagate the typo. However, according to the rules, we should not comment on things that would be caught by the build system, including import issues. The file might actually exist with the misspelled name, in which case this is a valid spelling issue that won't be caught by the build. However, if the file doesn't exist, the build will catch it. Since we can't verify if the misspelled file exists or not without more context, and the rules explicitly say to not comment on import issues, we should err on the side of removing the comment. Delete the comment since it relates to an import statement, which falls under the rule about not commenting on things that would be caught by the build system.

Workflow ID: wflow_ZZccNZtFxg8iTxby

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 2d47bd6 in 1 minute and 5 seconds. Click for details.
  • Reviewed 26 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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/openapi.yaml:8455
  • Draft comment:
    Improved summary text for consistency by using 'HTTP triggers'.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment seems to be purely informative, as it is just acknowledging an improvement in the summary text for consistency. It does not provide any actionable feedback or suggestions for improvement.
2. frontend/src/lib/components/triggers/http/HttpTriggersGenerator.svelte:113
  • Draft comment:
    Fixed typo in error message: 'occured' to 'occurred'.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, as it only points out a typo correction in an error message. It does not provide any actionable feedback or suggestions for improvement.

Workflow ID: wflow_cujzkxZ2MwTyTVsJ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed c471e9f in 2 minutes and 6 seconds. Click for details.
  • Reviewed 18 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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/FolderPicker.svelte:31
  • Draft comment:
    The filter no longer excludes 'app_groups'. Ensure this removal is intentional.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/lib/components/FolderPicker.svelte:31
  • Draft comment:
    Duplicate condition for 'app_custom' exists; remove redundant check.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_YYuT1c2p1cY9DmCE

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed 3fd2bcd in 2 minutes and 5 seconds. Click for details.
  • Reviewed 39 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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/Path.svelte:22
  • Draft comment:
    Removed extraneous blank line in import block for cleaner formatting.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. frontend/src/lib/components/Path.svelte:170
  • Draft comment:
    Simplified filter: removed duplicate exclusion of 'app_custom'. Ensure filtering logic matches intended exclusions.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_YweWIxssr5YUeQ80

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed c4c0da0 in 1 minute and 9 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft 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/http/HttpTriggersGenerator.svelte:129
  • Draft comment:
    The button size was changed from 'sm' to 'xs'. Ensure this smaller size still meets our accessibility and UI guidelines across devices.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that the change meets accessibility and UI guidelines, which is against the rules. It does not provide a specific suggestion or ask for confirmation of an intentional change.

Workflow ID: wflow_0Hf1WI6Z0RGEpjJZ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 66f3410 in 1 minute and 44 seconds. Click for details.
  • Reviewed 230 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft 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/FolderPicker.svelte:21
  • Draft comment:
    Consider explicitly defaulting the boolean props (disabled and disableEditing) to false instead of leaving them undefined. This helps avoid unexpected truthy/falsy behavior.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. frontend/src/lib/components/FolderPicker.svelte:118
  • Draft comment:
    Use optional chaining (e.g. newFolder?.openDrawer()) when calling the openDrawer method to guard against a potential null reference.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/lib/components/Path.svelte:424
  • Draft comment:
    Replacing the inline folder management UI with the FolderPicker component improves separation of concerns. Verify that FolderPicker fully replicates the previous functionality (both viewing and creating folders) as originally implemented.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify that the new implementation of FolderPicker replicates the previous functionality. This falls under the rule of not asking the author to confirm their intention or to ensure behavior is intended. Therefore, this comment should be removed.
4. frontend/src/lib/components/Path.svelte:33
  • Draft comment:
    Typo alert: The path './random_positive_adjetive' seems to have a misspelling. Did you mean './random_positive_adjective'?
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_a95NYqcKpb2XlpNY

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@HugoCasa HugoCasa changed the title Dieri/generate http route triggers from openapi spec feat: generate http route triggers from openapi spec Jun 4, 2025
Copy link
Collaborator

@HugoCasa HugoCasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you include screenshots in your PR?

&w_id,
None,
ct.workspaced_route,
new_http_trigger.workspaced_route,
&db,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's quite important that the route check is done on the transaction rather than on db

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 1dc4dbb in 1 minute and 10 seconds. Click for details.
  • Reviewed 305 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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/http/HttpTriggersGenerator.svelte:101
  • Draft comment:
    The catch block in saveHttpTrigger still uses 'error.body'. Use 'error.message || error' for consistency and to avoid undefined errors.
  • 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. frontend/src/lib/components/triggers/http/HttpTriggersGenerator.svelte:90
  • Draft comment:
    Renamed service call from 'createManyHttpTrigger' to 'createHttpTriggers'. Ensure the backend API and type definitions are updated to match this new naming.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that the backend API and type definitions are updated to match the new naming. This falls under the category of asking the author to ensure something is done, which is against the rules.

Workflow ID: wflow_7Slouj4cf7BFJXeI

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 355b1a2 in 2 minutes and 38 seconds. Click for details.
  • Reviewed 294 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 draft 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/http_triggers.rs:300
  • Draft comment:
    Passing 'new_http_trigger' by value (instead of a reference) transfers ownership. Confirm this change is intentional and acceptable for reuse downstream.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. backend/windmill-api/src/http_triggers.rs:308
  • Draft comment:
    Consider enhancing the 'Invalid route path' error message with details on the expected format to aid debugging.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. backend/windmill-api/src/http_triggers.rs:404
  • Draft comment:
    handle_deployment_metadata is invoked within the transaction. Verify that any external calls here won’t introduce transaction-related issues.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify that external calls within a transaction won't introduce issues. This falls under asking the author to double-check things, which is against the rules.
4. frontend/src/lib/components/triggers/http/HttpRoutesGenerator.svelte:124
  • Draft comment:
    UI labels updated from 'HTTP triggers' to 'HTTP routes' are consistent. Ensure all user-facing strings reflect the new terminology.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
5. frontend/src/lib/components/triggers/http/HttpRoutesGenerator.svelte:40
  • Draft comment:
    The use of 'forceRerender' to trigger UI updates is a workaround; consider a more reactive pattern if possible.
  • 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/routes/(root)/(logged)/routes/+page.svelte:47
  • Draft comment:
    HttpRoutesGenerator is imported now instead of the old trigger component. Confirm that the integration (APIs and props) remains compatible.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
7. frontend/src/routes/(root)/(logged)/routes/+page.svelte:310
  • Draft comment:
    Ensure the 'getHttpRoute' function call with parameter 'r' returns the correct URL format for HTTP routes under the new naming.
  • 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. backend/windmill-api/src/http_triggers.rs:433
  • Draft comment:
    Typo: The success message uses "Created all http triggers" with a lowercase 'http'. For consistency with other parts of the code (where 'HTTP' is capitalized), consider changing it to "Created all HTTP triggers".
  • 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 inconsistency, this is an extremely minor cosmetic issue that doesn't affect functionality. The message is only seen in logs/responses and doesn't impact code quality or behavior. The rules say not to make comments that are obvious or unimportant. This seems to fall into that category. The comment does point out a real inconsistency in the codebase. Consistent capitalization could be considered part of code quality. While consistency is good, this is too minor of an issue to warrant a PR comment. The benefit of fixing this is extremely small compared to the overhead of a PR comment and review cycle. Delete this comment as it points out an unimportant cosmetic issue that doesn't meaningfully impact code quality.

Workflow ID: wflow_Fb2rJRJh1l4P5P1R

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@dieriba dieriba force-pushed the dieri/generate-http-route-triggers-from-openapi-spec branch from 355b1a2 to 258f945 Compare June 5, 2025 09:02
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed 514d5ae in 2 minutes and 45 seconds. Click for details.
  • Reviewed 543 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft 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/http/HttpTriggersGenerator.svelte:304
  • Draft comment:
    In the callback for the Edit button, the code reassigns httpTriggers to itself in order to trigger reactivity. While this pattern can work, consider adding a comment or using a more explicit reactive method to improve readability.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. backend/windmill-api/src/http_triggers.rs:375
  • Draft comment:
    Possible typo: in the tuple being inserted into the HashSet, the field trigger.workspaced_route looks suspicious. Did you mean trigger.workspace_route?
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. frontend/src/lib/components/triggers/http/HttpTriggersGenerator.svelte:175
  • Draft comment:
    Typographical error: The else block contains a bare 'openApiUrlEditorValue' without any assignment operator. Likely, it was intended to assign it like the other cases (e.g., 'openApiUrlEditorValue = code').
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_j72TanPdZFtCgYM7

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 03fd1c0 in 43 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft 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/http/HttpTriggersGenerator.svelte:175
  • Draft comment:
    Corrected bug: The assignment openApiUrlEditorValue = code is now correctly updating the variable for URL mode. Previously, the missing assignment operator caused a logical error.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, explaining a bug fix that was made. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. According to the rules, purely informative comments should be removed.

Workflow ID: wflow_2HYif4K2gNtIsG5m

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 6f2e563 in 3 minutes and 13 seconds. Click for details.
  • Reviewed 227 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft 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/http_triggers.rs:377
  • Draft comment:
    Using indexed access to the route_path_keys array requires that it exactly matches the order and length of new_http_triggers. Ensure that the array is always in sync to avoid mismatches.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is suggesting to ensure that two arrays are in sync, which is a form of asking the author to double-check their work. This violates the rule against asking the author to ensure behavior is intended or to double-check things.
2. frontend/src/lib/components/triggers/http/HttpTriggersGenerator.svelte:145
  • Draft comment:
    The UI layout has been refactored (e.g. removal of Splitpanes and updated button text to 'Save routes'). Verify that the new simpler structure meets the responsive design and UX expectations.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. frontend/src/routes/(root)/(logged)/routes/+page.svelte:347
  • Draft comment:
    Updated toast message now reads 'Successfully deleted HTTP route' which improves consistency. Confirm that this change is reflected throughout the UI.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
4. frontend/src/lib/components/triggers/http/HttpTriggersGenerator.svelte:272
  • Draft comment:
    There seems to be a typographical error on the closing tag for the Button component. The closing tag is split across two lines ("</Button" on line 272 and ">" on line 273). Please combine these into a single syntactically correct </Button> tag.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_CgfsPScOsZVrZ29Y

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed e8eafa8 in 1 minute and 49 seconds. Click for details.
  • Reviewed 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft 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/vite.config.js:48
  • Draft comment:
    Ensure the additional globals and protocolImports options are intentional and needed. Also, review the indentation/style to match the rest of the config.
  • 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 comment violates several rules. It asks to "ensure" options are intentional, which is a form of asking for confirmation - this is explicitly forbidden. The indentation issue would be caught by linting/formatting tools. We don't have enough context to know if the polyfill options are actually problematic or incorrect. The indentation issue is real and visible. Maybe inconsistent formatting could cause real problems in some build systems? No - formatting issues should be handled by automated tools, not PR comments. The rules explicitly say not to comment on things that would be caught by the build. The comment should be removed as it primarily asks for confirmation of intent and points out formatting issues that should be handled by automated tools.

Workflow ID: wflow_8K2knMuPP2fnHNjE

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed 6cd6d02 in 55 seconds. Click for details.
  • Reviewed 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft 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/vite.config.js:48
  • Draft comment:
    Removed extra nodePolyfills options (globals & protocolImports) – confirm this change is intentional since it may break polyfills for Buffer, global, or process.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_AMgtUNDSdwB2Evf7

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@HugoCasa HugoCasa added this pull request to the merge queue Jun 5, 2025
Merged via the queue into main with commit 5713483 Jun 5, 2025
16 checks passed
@HugoCasa HugoCasa deleted the dieri/generate-http-route-triggers-from-openapi-spec branch June 5, 2025 22:46
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2025
@dieriba dieriba restored the dieri/generate-http-route-triggers-from-openapi-spec branch June 6, 2025 06:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants