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: websocket trigger allow returning messages #5168

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

HugoCasa
Copy link
Contributor

@HugoCasa HugoCasa commented Jan 29, 2025

Important

Add support for WebSocket triggers to return messages, including backend logic and frontend UI updates.

  • Backend:
    • Add can_return_message column to websocket_trigger table in migrations/20250129094837_websocket_can_return_message.up.sql and migrations/20250129094837_websocket_can_return_message.down.sql.
    • Update run_wait_result_internal() in jobs.rs to return a tuple with result and success status.
    • Modify create_websocket_trigger() and update_websocket_trigger() in websocket_triggers.rs to handle can_return_message.
    • Add logic in listen_to_websocket() in websocket_triggers.rs to handle message sending back to WebSocket server.
  • Frontend:
    • Add can_return_message toggle in WebsocketTriggerEditorInner.svelte and WebsocketEditorConfigSection.svelte.
    • Update labels and descriptions in various Svelte components to reflect WebSocket changes.
    • Ensure UI components like ToggleButton and Dropdown are updated to support new WebSocket functionality.

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

@HugoCasa HugoCasa requested a review from rubenfiszel as a code owner January 29, 2025 15: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.

👍 Looks good to me! Reviewed everything up to 65e3946 in 3 minutes and 0 seconds

More details
  • Looked at 1767 lines of code in 19 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/routes/(root)/(logged)/websocket_triggers/+page.svelte:225
  • Draft comment:
    Consistently use 'WebSocket' instead of 'websocket' for clarity and correctness. This applies to other instances in the code as well.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/routes/(root)/(logged)/websocket_triggers/+page.svelte:329
  • Draft comment:
    Consistently use 'WebSocket' instead of 'websocket' for clarity and correctness. This applies to other instances in the code as well.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_A6NQKIEw7ruAxUuT


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

Copy link

cloudflare-workers-and-pages bot commented Jan 29, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 167de6c
Status: ✅  Deploy successful!
Preview URL: https://8cc4ebf1.windmill.pages.dev
Branch Preview URL: https://hugo-win-876-sending-websock.windmill.pages.dev

View logs

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 167de6c in 42 seconds

More details
  • Looked at 388 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. backend/windmill-api/src/jobs.rs:3606
  • Draft comment:
    Consider improving error handling in run_wait_result_internal for clarity and consistency. Currently, it returns an error message directly, which might not be consistent with other parts of the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new function run_wait_result_internal which is used in multiple places. The function run_wait_result now calls run_wait_result_internal. This refactoring is good for code reuse and separation of concerns. However, the error handling in run_wait_result_internal could be improved for clarity and consistency.
2. backend/windmill-api/src/websocket_triggers.rs:604
  • Draft comment:
    Ensure that the SQL query for fetching early_return is optimized for performance. Consider indexing the flow_version table appropriately and using the array_upper function efficiently.
  • Reason this comment was not posted:
    Confidence changes required: 40%
    The PR introduces a SQL query to fetch early_return from the flow_version table. This query is used in multiple places. However, the query could be optimized by ensuring that the array_upper function is used efficiently, and the query is indexed properly for performance.

Workflow ID: wflow_hBMQey6ZlwQhwn6i


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

@rubenfiszel rubenfiszel merged commit 487c273 into main Jan 29, 2025
8 checks passed
@rubenfiszel rubenfiszel deleted the hugo/win-876-sending-websocket-messages-back branch January 29, 2025 20:17
@github-actions github-actions bot locked and limited conversation to collaborators Jan 29, 2025
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