-
Notifications
You must be signed in to change notification settings - Fork 306
[feat] Add auto-refresh on task completion for RemoteWidget nodes #4191
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
Conversation
d2bb38c
to
d7f8834
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits.
Overall for the current solution, it feels a little over-engineered. Assuming re-use and that we will add more complex, fine-tuned UX as our app matures in the near future, it feels awesome.
let taskWatcher: ReturnType<typeof useTaskCompletionWatcher> | null = null | ||
|
||
// Initialize task completion watcher | ||
taskWatcher = useTaskCompletionWatcher({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is just immediate reassignment?
let taskWatcher: ReturnType<typeof useTaskCompletionWatcher> | null = null | |
// Initialize task completion watcher | |
taskWatcher = useTaskCompletionWatcher({ | |
// Initialize task completion watcher | |
const taskWatcher = useTaskCompletionWatcher({ |
taskWatcher?.start() | ||
} else { | ||
taskWatcher?.stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taskWatcher?.start() | |
} else { | |
taskWatcher?.stop() | |
taskWatcher.start() | |
} else { | |
taskWatcher.stop() |
|
||
// Cleanup on node removal | ||
node.onRemoved = useChainCallback(node.onRemoved, function () { | ||
taskWatcher?.stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taskWatcher?.stop() | |
taskWatcher.stop() |
onComplete: () => { | ||
if (widget.refresh) { | ||
widget.refresh() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onComplete: () => { | |
if (widget.refresh) { | |
widget.refresh() | |
} | |
} | |
onComplete: () => widget.refresh?.() |
start() | ||
} | ||
|
||
onUnmounted(() => stop()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onUnmounted(() => stop()) | |
onUnmounted(stop) |
nit: perf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a task completion watcher, let's simplify this PR by just listening for the execution_success
WS message.
As an example, look here:
Does this only need to run for output nodes? |
It should run when the workflow is done executing. |
Implements auto-refresh functionality that triggers when tasks complete for any RemoteWidget node. Users can toggle the feature on/off via a boolean widget labeled "Auto-refresh on task completion". Key components: - useTaskCompletionWatcher: Reusable composable that watches queue history to detect task completion - RemoteWidget enhancement: Automatically adds toggle widget to all remote widgets - Comprehensive test coverage for both composables Fixes #4153
…ctations - Changed default value from true to false - Added 'on' and 'off' labels for the toggle states - Ensures consistency with existing widget patterns
- Removed expectation for 'on' and 'off' properties in test - Aligns test with actual implementation after label removal
…vent - Remove complex task completion watcher composable - Use WebSocket 'execution_success' event for workflow completion detection - Much simpler implementation (~35 lines vs ~100+ lines) - Update widget label to "Auto-refresh after generation" - Add debug logging for execution success handler
- Update test to expect "Auto-refresh after generation" label - All tests now passing
c238abf
to
500b84c
Compare
Co-authored-by: filtered <176114999+webfiltered@users.noreply.github.com>
Co-authored-by: filtered <176114999+webfiltered@users.noreply.github.com>
Implements auto-refresh toggle button for nodes with remote widgets. When enabled, automatically refreshes the widget when the workflow completes execution.
auto-refresh-node-demo.mp4
Implementation Details:
execution_success
event to detect workflow completionTest Plan:
Fixes #4153
┆Issue is synchronized with this Notion page by Unito