Skip to content
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

feat(frontend): replace MenuV2 with melt Menu (1/4) #5214

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

Guilhem-lm
Copy link
Contributor

@Guilhem-lm Guilhem-lm commented Feb 5, 2025

Components affected by change of MenuV2:

  • SideBarContent
  • FavoriteMenu
  • OperatorMenu
  • WorkspaceMenu
  • UserMenu
  • AppMenu
    • Inside conditional Tab
    • Inside Modal
    • Inside List
    • Inside Drawer
    • Carousel list
  • FlowJobsMenu
  • page : svix/create-webhook
  • layout : sidebar

Most of the changes do not affect the UI. New features are :

  • Keyboard navigation inside the menu and between menus of the same menubar
  • Nit polishing of the User menu
  • Nit polishing of the operator menu
    Before :
    image
    After:
    image

Important

Replace MenuV2 with new melt menu components across the frontend, adding keyboard navigation and UI improvements.

  • Behavior:
    • Replace MenuV2 with Menubar, Menu, MenuItem, and MeltButton in AppMenu.svelte, FlowJobsMenu.svelte, and FavoriteMenu.svelte.
    • Add keyboard navigation support for menus.
    • Minor UI polishing for User and Operator menus.
  • Components:
    • Add new components Menubar.svelte, Menu.svelte, MenuItem.svelte, MeltButton.svelte in meltComponents.
    • Update MenuButton.svelte and MenuLink.svelte to use conditionalMelt for conditional rendering.
  • Misc:
    • Update +layout.svelte and create-webhook/+page@(root).svelte to use new menu components.
    • Add conditionalMelt function to utils.ts for conditional rendering of melt elements.

This description was created by Ellipsis for 68de430. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented Feb 5, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 68de430
Status: ✅  Deploy successful!
Preview URL: https://76e20dd8.windmill.pages.dev
Branch Preview URL: https://glm-replace-menuv2.windmill.pages.dev

View logs

# Conflicts:
#	frontend/src/lib/components/sidebar/WorkspaceMenu.svelte
@Guilhem-lm Guilhem-lm marked this pull request as ready for review February 11, 2025 12:13
@rubenfiszel rubenfiszel force-pushed the main branch 7 times, most recently from 3a05600 to 3b6585a Compare February 18, 2025 00:54
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.

👍 Looks good to me! Incremental review on 70a016e in 4 minutes and 27 seconds

More details
  • Looked at 7774 lines of code in 124 files
  • Skipped 2 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte:190
  • Draft comment:
    BUG: In the else branch of editWorkspaceCommand, the code always calls WorkspaceService.editSlackCommand. When platform is 'teams', it should call WorkspaceService.editTeamsCommand (using updateCommandScript) to remove the teams command script.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. python-client/wmill/wmill/client.py:1078
  • Draft comment:
    New method send_teams_message is added. Consider adding corresponding documentation and unit tests to cover its behavior.
  • 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 code change - the addition of a new method. However, the method already has basic documentation. Regarding tests, while testing is good practice, this comment is too vague to be actionable. It doesn't specify what scenarios should be tested or what might go wrong. The method is also a simple wrapper around an API call, which limits what can be meaningfully tested without mocking.
    I might be undervaluing the importance of comprehensive testing. Even simple API wrappers can have edge cases around parameter validation or error handling that should be tested.
    While testing is valuable, this comment doesn't provide specific guidance about what to test. A more actionable comment would point out specific scenarios or edge cases that need test coverage.
    The comment should be deleted because it's too vague to be actionable, and the documentation request is already satisfied by the existing docstring.
3. typescript-client/package.json:4
  • Draft comment:
    Version bump to 1.463.5 seems appropriate; please ensure all corresponding client libraries are updated accordingly.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte:190
  • Draft comment:
    BUG: In the removal branch of editWorkspaceCommand (lines 190-194), WorkspaceService.editSlackCommand is always called, regardless of the platform. It should call updateCommandScript so that when platform is 'teams', the correct API (editTeamsCommand) is used.
  • Reason this comment was not posted:
    Marked as duplicate.
5. openflow.openapi.yaml:4
  • Draft comment:
    INFO: OpenFlow spec is updated to version 1.463.5. The schema definitions appear consistent and follow best practices.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
6. python-client/wmill/wmill/client.py:711
  • Draft comment:
    INFO: The newly added send_teams_message method is implemented correctly using self.post('/teams/activities', ...). Ensure that the backend endpoint '/teams/activities' is available and properly secured.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
7. typescript-client/client.ts:40
  • Draft comment:
    INFO: The TypeScript client file properly sets up services and environment variables (getEnv, setClient). The implementation looks clean and follows best practices.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
8. typescript-client/package.json:1
  • Draft comment:
    INFO: Package version has been bumped to 1.463.5 and dependencies look correctly updated.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_KirHq2tTV3IrNfkH


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on af186df in 2 minutes and 12 seconds

More details
  • Looked at 607 lines of code in 20 files
  • Skipped 1 files when reviewing.
  • Skipped posting 25 drafted comments based on config settings.
1. CHANGELOG.md:1
  • Draft comment:
    Version bump section appears consistent with new version 1.463.6. Please ensure the referenced commits explain the updates clearly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to ensure that the referenced commits explain the updates clearly. This falls under the rule of not asking the author to confirm or ensure something, which is not allowed.
2. backend/Cargo.lock:1007
  • Draft comment:
    Version bumps in Cargo.lock are consistent with 1.463.6. Confirm that checksums and dependency versions are updated as intended.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to confirm that checksums and dependency versions are updated as intended, which violates the rule against asking for confirmation of intention. It also comments on dependency changes, which is not allowed.
3. backend/Cargo.toml:1
  • Draft comment:
    Version updated to 1.463.6 in Cargo.toml. Everything seems consistent.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and does not provide any actionable feedback or suggestions. It simply states that the version update seems consistent, which is not necessary for the PR author to know.
4. backend/windmill-api/openapi.yaml:4
  • Draft comment:
    API version updated to 1.463.6 in the OpenAPI YAML. Ensure that this change is in sync with backend API expectations.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is related to a version update in the OpenAPI YAML file. It asks the author to ensure that the change is in sync with backend API expectations, which is a form of asking for confirmation or verification. This violates the rule against asking the PR author to confirm or ensure behavior. Therefore, this comment should be removed.
5. backend/windmill-worker/nsjail/run.bash.config.proto:18
  • Draft comment:
    Mount points include /proc/self/fd now. Verify that the added mounts are safe and needed for the new 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.
6. backend/windmill-worker/nsjail/run.powershell.config.proto:24
  • Draft comment:
    The /proc/self/fd mount addition in the PowerShell config appears correct. Confirm there are no related security 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 confirm that there are no related security issues, which violates the rule against asking the author to confirm their intention or ensure behavior. It does not provide a specific suggestion or point out a specific issue.
7. backend/windmill-worker/src/bash_executor.rs:110
  • Draft comment:
    Using process substitution with tee (i.e. tee >(tail -1 >> ./result2.out)) avoids using a named pipe; this is a neat improvement. Ensure this behavior is correctly tested across environments.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is asking the PR author to ensure that a specific behavior is tested across environments. This falls under the rule of not asking the author to ensure that the change is tested, which is not allowed.
8. benchmarks/lib.ts:5
  • Draft comment:
    Benchmark version constant updated to v1.463.6; looks good.
  • 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 states that a version constant has been updated. It does not provide any actionable feedback or suggestions for improvement.
9. cli/main.ts:65
  • Draft comment:
    CLI version updated to 1.463.6. Nice consistency across different components.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it simply states that the CLI version has been updated and is consistent across components. It does not provide any actionable feedback or suggestions.
10. frontend/package.json:1
  • Draft comment:
    Frontend package version updated to 1.463.6, matching the new release. Confirm that build scripts work as expected with this version.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is related to a dependency change, specifically a version update for a frontend package. It asks the PR author to confirm that build scripts work as expected with this version, which violates the rule against asking for confirmation or verification of behavior. Therefore, this comment should be removed.
11. frontend/src/lib/utils.ts:1103
  • Draft comment:
    New utility function isNotFlow() provides clearer logic for checking non-flow job types. This enhances code readability.
  • 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 praises the new utility function for enhancing code readability without providing any actionable feedback or suggestions. According to the rules, purely informative comments should be removed.
12. frontend/src/routes/(root)/(logged)/run/[...run]/+page.svelte:123
  • Draft comment:
    Guard for fetching logs now includes isNotFlow(job?.job_kind) which refines when getLogs is invoked. This prevents inappropriate log fetches for flow jobs.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, explaining a change made in the code without suggesting any action or asking for clarification. It doesn't align with the rules for useful comments, as it doesn't provide a suggestion, ask for confirmation of intent, or highlight a potential issue.
13. lsp/Pipfile:7
  • Draft comment:
    Pipfile now requires wmill and wmill_pg versions >= 1.463.6. This maintains consistency with backend changes.
  • 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 states the change in the Pipfile and its purpose. It does not provide any actionable feedback or suggestions for improvement.
14. openflow.openapi.yaml:4
  • Draft comment:
    OpenFlow spec version updated to 1.463.6; this fits with overall version management.
  • 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 states that the OpenFlow spec version has been updated and fits with overall version management. It does not provide any actionable feedback or suggestions for improvement.
15. powershell-client/WindmillClient/WindmillClient.psd1:15
  • Draft comment:
    ModuleVersion bumped to 1.463.6 in the PSD1 file. Verify that the exported functions and manifest GUID remain valid.
  • 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 the validity of exported functions and manifest GUID after a module version bump. This falls under asking the author to double-check things, which is against the rules.
16. python-client/wmill/pyproject.toml:3
  • Draft comment:
    Python client version updated to 1.463.6. All metadata, like license and homepage, are consistent.
  • 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 states that the Python client version was updated and that metadata is consistent. It doesn't provide any actionable feedback or suggestions for improvement.
17. python-client/wmill_pg/pyproject.toml:3
  • Draft comment:
    Extension client version bumped to 1.463.6 and wmill dependency updated accordingly. Looks correct.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it just states that a version bump and dependency update have occurred. It doesn't provide any actionable feedback or suggestions for improvement.
18. typescript-client/jsr.json:3
  • Draft comment:
    TypeScript client package version updated to 1.463.6. Ensure exports and publish settings are correct.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is related to a dependency version change, which is not allowed according to the rules. It also asks the author to ensure settings are correct, which is not specific enough to be useful.
19. typescript-client/package.json:4
  • Draft comment:
    Windmill CLI client version updated to 1.463.6. Prepublish script still triggers TypeScript compilation.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, mentioning a version update and a script trigger without suggesting any action or raising any concern. It doesn't provide any actionable feedback or question.
20. version.txt:1
  • Draft comment:
    Version.txt updated to 1.463.6; this file is now in sync with other version bumps.
  • 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 states that a version file has been updated and is in sync with other version bumps. It doesn't provide any actionable feedback or suggestions for improvement.
21. python-client/wmill_pg/pyproject.toml:19
  • Draft comment:
    The dependency for 'wmill' is set as '^1.5.0', but the main client uses version '1.463.6'. This versioning appears inconsistent—please verify that this is intentional or update it 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.
22. frontend/src/routes/(root)/(logged)/run/[...run]/+page.svelte:123
  • Draft comment:
    The reactive statement now includes an additional condition (isNotFlow) to avoid fetching logs for flow jobs. Confirm that this extra check meets the desired UX behavior for flows.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
23. powershell-client/WindmillClient/WindmillClient.psd1:15
  • Draft comment:
    Module version updated to '1.463.6' is consistent with the current release. Verify that all metadata (GUID, license, etc.) remain up to date.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
24. typescript-client/package.json:4
  • Draft comment:
    Version in TypeScript client updated to '1.463.6' is in line with release changes.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
25. version.txt:1
  • Draft comment:
    The version file is correctly updated to '1.463.6'.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_StVAkI2slQtjGFVL


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on f7b7ff0 in 1 minute and 36 seconds

More details
  • Looked at 62 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. frontend/src/lib/components/sidebar/OperatorMenu.svelte:115
  • Draft comment:
    Consider using the browser's setTimeout return type 'number' instead of NodeJS.Timeout. This may avoid type mismatches in a DOM environment.
  • 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 setTimeout does return a number in browsers, NodeJS.Timeout is also valid and more explicit about the intent. The code works fine either way. This seems like a minor type preference rather than a real issue. The current type is not wrong or problematic.
    The comment could be correct that number is more idiomatic in browser code, but the current type is also valid and works perfectly fine. This is more of a style preference than a real issue.
    Since both types work correctly and this is just a style preference without any real impact on functionality or maintainability, this comment doesn't meet the bar for being an important enough issue to raise.
    Delete this comment as it suggests a change that is purely preferential and doesn't fix any actual problems.
2. frontend/src/lib/components/sidebar/OperatorMenu.svelte:117
  • Draft comment:
    Clear any pending timeout on component unmount with onDestroy to prevent side effects or memory leaks.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, suggesting a best practice without directly addressing a specific issue in the code. It doesn't ask for a change or clarification, nor does it point out a potential problem with the current implementation. Therefore, it violates the rule against making purely informative comments.
3. frontend/src/lib/components/sidebar/OperatorMenu.svelte:115
  • Draft comment:
    Consider using a browser-appropriate timeout type (e.g., number) instead of NodeJS.Timeout to avoid potential type mismatches.
  • Reason this comment was not posted:
    Marked as duplicate.
4. frontend/src/lib/components/sidebar/OperatorMenu.svelte:117
  • Draft comment:
    Clear pending timeouts on component destruction (using onDestroy) to prevent state updates after unmounting.
  • Reason this comment was not posted:
    Marked as duplicate.
5. frontend/src/lib/components/sidebar/OperatorMenu.svelte:122
  • Draft comment:
    Consider extracting the 150ms debounce delay into a named constant for improved readability and easier tuning.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_yNpJ9xokHWJHf7qN


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on 8ed283f in 2 minutes and 3 seconds

More details
  • Looked at 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/lib/components/meltComponents/Menu.svelte:71
  • Draft comment:
    Consider potential Tailwind purging issues when using a dynamic arbitrary class (z-[${zIndex}]). Ensure that the generated class (e.g., z-[100]) is included in your Tailwind configuration or consider reverting to an inline style for dynamic values.
  • 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 raises a valid technical concern about Tailwind's purging behavior with dynamic arbitrary values. However, since zIndex is a constant from zIndexes.contextMenu, not a truly dynamic value, this likely won't be an issue in practice. The build process would catch any purging problems immediately. The comment seems overly cautious and not clearly actionable.
    I might be underestimating the risk of Tailwind purging issues. There could be edge cases where the zIndex value changes that I'm not seeing.
    Since zIndex is a constant and any purging issues would be caught by the build process immediately, this is not a significant concern that requires a comment.
    Delete this comment as it raises a theoretical issue that would be caught by the build process, and the value is actually a constant, not truly dynamic.
2. frontend/src/lib/components/meltComponents/Menu.svelte:46
  • Draft comment:
    Ensure zIndexes.contextMenu is a numeric value. If it could be a string, the tailwind class generated (z-[value]) might not work as expected.
  • 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%
  1. Tailwind's z-[value] syntax actually works with both numbers and strings. 2. The previous code used the value directly in a style attribute which also worked fine. 3. This suggests zIndexes.contextMenu is already in a compatible format. 4. Without seeing zIndexes.ts, we can't be 100% sure of the type, but the previous working code is strong evidence it's already correct.
    We haven't seen the zIndexes.ts file, so we don't know for certain what type contextMenu is. The previous code working isn't absolute proof it will work in the new context.
    While we don't have absolute certainty, both the previous working code and Tailwind's flexibility with z-[value] make this comment unnecessarily cautious. If there was a type issue, TypeScript would likely catch it.
    The comment should be deleted as it's overly cautious and suggests a fix for a problem that likely doesn't exist, given the evidence from the previous working code.
3. frontend/src/lib/components/meltComponents/Menu.svelte:71
  • Draft comment:
    Using a dynamic tailwind class (z-[${zIndex}]) may be missed by Tailwind's purge. Verify that this value is properly safelisted or static for production builds.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_WtkNNCUUxi4pOCUv


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@Guilhem-lm Guilhem-lm changed the title feat(frontend): replace MenuV2 with melt Menu feat(frontend): replace MenuV2 with melt Menu (1/4) Feb 19, 2025
pyranota and others added 3 commits February 19, 2025 16:33
* refactor!: Remove `pip` fallback option for python and ansible

BREAKING CHANGE: pip was deprecated since 1.425.0 (2024-11-15)

* fix errors in main.rs

* fix tests

* remove nsjail for pip

* fix imports

* fix compilation error
# Conflicts:
#	frontend/src/lib/components/sidebar/SidebarContent.svelte
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.

❌ Changes requested. Incremental review on 6786399 in 2 minutes and 15 seconds

More details
  • Looked at 706 lines of code in 15 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. frontend/src/lib/utils.ts:602
  • Draft comment:
    Extra closing brace in customPlural case. Consider removing the extra '}' in the return string.
  • 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/meltComponents/MeltButton.svelte:10
  • Draft comment:
    Check the use of '$meltElement'. If meltElement is not a store, remove the '$' prefix to correctly apply the melt action.
  • 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 type AnyMeltElement comes from @melt-ui/svelte, and without knowing the library's internals, we can't be certain whether the $ prefix is incorrect. The author may be following the library's intended usage. Making this change could break functionality if meltElement is meant to be a store.
    I might be missing documentation or common patterns from the Melt UI library that would clarify whether meltElement should be accessed as a store.
    Without strong evidence that the $ prefix is incorrect, and given this is a new component using a specific UI library, we should err on the side of assuming the author implemented it correctly.
    Delete the comment as we don't have strong evidence that it's incorrect, and changing it could potentially break functionality.
3. frontend/src/lib/utils.ts:600
  • Draft comment:
    Extra closing brace in the customPlural branch of the pluralize function. Remove the extra '}' to correctly format the 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.
4. frontend/src/lib/utils.ts:967
  • Draft comment:
    Using the Function constructor in evalJs can lead to security concerns. Ensure expressions passed here are properly sanitized to avoid code injection.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/src/lib/components/flows/map/FlowJobsMenu.svelte:68
  • Draft comment:
    Typo Alert: The id attribute is currently set as flow-editor-iteration picker-${id} with an unintended space between 'iteration' and 'picker'. HTML id values should not contain spaces. Consider updating it to something like flow-editor-iteration-picker-${id}.
  • Reason this comment was not posted:
    Comment was on unchanged code.
6. frontend/src/lib/components/sidebar/MenuButton.svelte:53
  • Draft comment:
    Typographical/lexicographical inconsistency: It appears that the spread operator has been changed from {...trigger} to {...$trigger}. However, the trigger prop is declared as a regular prop (MenubarMenuElements['trigger'] | undefined), not a Svelte store. Using $trigger (which usually refers to store values) might be incorrect or inconsistent with the rest of the usage (e.g., in use:conditionalMelt={trigger}). Please review if the $ prefix is intended. If not, revert it to {...trigger}.
  • Reason this comment was not posted:
    Marked as duplicate.
7. frontend/src/lib/components/sidebar/OperatorMenu.svelte:218
  • Draft comment:
    Typo: The href attribute in the MenuItem for user workspaces is written as href="{base}/user/workspaces". In Svelte, this should be unquoted (e.g., href={base + '/user/workspaces'}) so that the base variable is properly interpolated.
  • 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%
    In Svelte, both href="{base}/user/workspaces" and href={base + '/user/workspaces'} would actually work correctly. The first version uses string interpolation within quotes, while the second uses string concatenation. Since both achieve the same result and the current code is working, this is more of a style preference than a functional issue.
    I might be overlooking some Svelte-specific best practices or performance implications of the different syntax approaches.
    Even if there are slight differences, both syntaxes are valid Svelte and would work correctly here. This is too minor of a style issue to warrant a comment.
    The comment should be deleted as it suggests changing working code to an equivalent alternative syntax, which is purely a style preference.
8. frontend/src/lib/utils.ts:602
  • Draft comment:
    Typo: In the pluralize function, there's an extra '}' in the template literal on line 602. It should be ${quantity} ${customPlural} rather than ${quantity} ${customPlural}}.
  • 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_vGTVnuapC96a3Eo8


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on 345dc30 in 2 minutes and 9 seconds

More details
  • Looked at 307 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 17 drafted comments based on config settings.
1. backend/.sqlx/query-08f288d2781d823e109a9e5b8848234ca7d1efeee9661f3901f298da375e73f7.json:130
  • Draft comment:
    Column reordering and renaming (e.g., 'teams_command_script' now becomes 'ai_models', etc.) may affect downstream dependencies. Verify that these changes are aligned with expected schema mappings.
  • 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 changes are aligned with expected schema mappings, which falls under asking the author to ensure the behavior is intended or to double-check things. This violates the rules.
2. backend/.sqlx/query-55cb03040bc2a8c53dd7fbb42bbdcc40f463cbc52d94ed9315cf9a547d4c89f2.json:130
  • Draft comment:
    Similar column reordering and renaming as in the other query file. Confirm that these adjustments correctly reflect the new spec and do not break backend queries.
  • Reason this comment was not posted:
    Marked as duplicate.
3. backend/.sqlx/query-ddf2eccb78a310ed00c7d8b9c3f05d394a7cbcf0038c72a78add5c7b02ef5927.json:18
  • Draft comment:
    The 'nullable' value was changed from true to null. Confirm this modification correctly represents the intended nullability semantics.
  • 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 confirm their intention regarding a change in nullability semantics. This violates the rule against asking the author to confirm their intention or ensure the behavior is intended. The comment does not provide a specific suggestion or point out a clear issue, so it should be removed.
4. backend/.sqlx/query-fec6d5674dc6b5a6a0ece419c40508835affcb7679a48f2a443777e829bd1e74.json:41
  • Draft comment:
    The nullable array values shifted from false to true for several columns. Please verify that this change aligns with the updated schema requirements.
  • 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 if the change aligns with schema requirements, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code.
5. backend/.sqlx/query-24178c21aadc1aed90f31e9362c6505a642c8f04b883c278b07e7ef5956ce121.json:1
  • Draft comment:
    Multiple legacy SQL query files have been removed (this file along with others). Ensure that all references in the backend code have been updated and that these removals are intentional.
  • 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-common/src/ee.rs:29
  • Draft comment:
    Added a new 'Teams' variant to CriticalErrorChannel along with the TeamsChannel struct. Ensure that serialization and downstream handling are updated accordingly.
  • 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 serialization and downstream handling are updated, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
7. backend/.sqlx/query-08f288d2781d823e109a9e5b8848234ca7d1efeee9661f3901f298da375e73f7.json:130
  • Draft comment:
    Renamed columns and updated types. Ensure corresponding consumer code and migrations are updated.
  • Reason this comment was not posted:
    Marked as duplicate.
8. backend/.sqlx/query-55cb03040bc2a8c53dd7fbb42bbdcc40f463cbc52d94ed9315cf9a547d4c89f2.json:130
  • Draft comment:
    Column renames and type updates consistent with new schema. Verify downstream mappings.
  • Reason this comment was not posted:
    Marked as duplicate.
9. backend/.sqlx/query-ddf2eccb78a310ed00c7d8b9c3f05d394a7cbcf0038c72a78add5c7b02ef5927.json:15
  • Draft comment:
    Nullable flag changed from 'true' to 'null' for the column. Confirm if this is intentional.
  • Reason this comment was not posted:
    Marked as duplicate.
10. backend/.sqlx/query-fec6d5674dc6b5a6a0ece419c40508835affcb7679a48f2a443777e829bd1e74.json:41
  • Draft comment:
    Nullable flags for non-nullable columns (indicated by '!') changed from false to true. This might cause mapping issues.
  • Reason this comment was not posted:
    Marked as duplicate.
11. backend/.sqlx/query-24178c21aadc1aed90f31e9362c6505a642c8f04b883c278b07e7ef5956ce121.json:1
  • Draft comment:
    Obsolete trigger-check query removed. Confirm it is no longer in use.
  • 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-4a804ee30bfe86c4e2c15a9f6511be5adf0dd22cb942fac64b439fb4e20df447.json:1
  • Draft comment:
    Obsolete metrics insertion query removed. Ensure no dependencies remain.
  • 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/.sqlx/query-65c339164e7669360d231d70105849e72bdc197c17c0fc51777c1dc9267e2daf.json:1
  • Draft comment:
    Removal of workspace_settings update query. Verify it is not needed 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.
14. backend/.sqlx/query-81b06122c7a12a314d8905ba5c7c14aa7614f2610e79a8c7302eaa63fb74984d.json:1
  • Draft comment:
    Obsolete global_settings update query removed. Ensure migration 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.
15. backend/.sqlx/query-e565f3b2e51059f563d18a8a9442bcae9640cee7b936820cb46c011222a77ff0.json:1
  • Draft comment:
    Removal of global_settings 'teams' update query confirmed. Verify no leftover references.
  • 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.
16. backend/.sqlx/query-ed318070b26861fda2d591a4356fdbeb6c7fdc965be43bddb010fd8299af1286.json:1
  • Draft comment:
    Obsolete metrics insertion query removed. Confirm removal is safe.
  • 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.
17. backend/windmill-common/src/ee.rs:29
  • Draft comment:
    Added Teams variant and TeamsChannel struct. Verify downstream integration for alerting and notifications.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_s3uspItaRLHsUI9e


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

👍 Looks good to me! Incremental review on 68de430 in 51 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/lib/components/apps/components/display/AppMenu.svelte:101
  • Draft comment:
    Removed on:close and on:open events. Confirm these events are no longer needed or handled elsewhere in the melt Menu replacement.
  • 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/apps/components/display/AppMenu.svelte:101
  • Draft comment:
    Removed 'on:close' and 'on:open' listeners. Ensure these events aren’t needed for menu interactions with the new melt Menu API.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_B4ZBHPa7JIttSbX4


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants