feat(workflows): Wire up wait-until-event handler and UI#59299
Open
dmarchuk wants to merge 4 commits into
Open
feat(workflows): Wire up wait-until-event handler and UI#59299dmarchuk wants to merge 4 commits into
dmarchuk wants to merge 4 commits into
Conversation
Contributor
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
nodejs/src/cdp/services/hogflows/actions/conditional_branch.test.ts:206-229
The first two `it` blocks both set `eventMatched = true`, call `handler.execute()` with identical inputs, then each assert only one thing. This means the handler is executed twice for what is really a single code path, violating the OnceAndOnlyOnce simplicity rule. Both assertions belong in one test.
```suggestion
it('advances to the matched branch and clears eventMatched', async () => {
waitInvocation.state.currentAction!.eventMatched = true
const result = await handler.execute({
invocation: waitInvocation,
action: waitAction,
result: createInvocationResult(waitInvocation),
})
expect(result.nextAction).toEqual(findActionById(waitInvocation.hogFlow, 'matched_target'))
expect(result.result).toEqual({ eventMatched: true })
expect(waitInvocation.state.currentAction!.eventMatched).toBe(false)
})
```
Reviews (1): Last reviewed commit: "Add wait-until-event UI behind workflows..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Size Change: 0 B Total Size: 79.8 MB ℹ️ View Unchanged
|
dmarchuk
force-pushed
the
feat/wait-until-event-handler
branch
from
fc5e6e1 to
b106053
Compare
dmarchuk
force-pushed
the
feat/hogflow-subscription-matcher
branch
from
5a56fb7 to
1f1e7b6
Compare
dmarchuk
force-pushed
the
feat/wait-until-event-handler
branch
4 times, most recently
from
eba4f33 to
2b4abb2
Compare
dmarchuk
force-pushed
the
feat/wait-until-event-handler
branch
from
8e8168e to
eeaedde
Compare
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/workflows/frontend/Workflows/hogflows/steps/StepWaitUntilCondition.tsx:51-58
When a user removes all entries from the event filter picker, `setFilters` receives `null` or `undefined`. The `?? {}` guard converts that to an empty object, so `events` is written as `[{ filters: {} }]` rather than cleared to `undefined`. Depending on how the subscription matcher interprets an empty filter object this could behave as "wake on every event" rather than "no event subscription".
```suggestion
<HogFlowEventFilters
filtersKey={`wait-until-events-${action.id}`}
filters={eventFilters}
setFilters={(newFilters) =>
partialSetWorkflowActionConfig(action.id, {
events: newFilters ? [{ filters: newFilters }] : undefined,
})
}
```
Reviews (2): Last reviewed commit: "Add wait-until-event matcher E2E test sc..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The hogflow subscription matcher (#57847) wakes parked
wait_until_conditionjobs and sets aneventMatchedflag when an incoming event matched a step's event subscription. Two pieces are still missing to make the feature usable:ConditionalBranchHandlerignoreseventMatchedand re-evaluates the stored property condition against the original triggering event, so an event-driven wait can never advance.wait_until_conditionstep should wait on.Changes
ConditionalBranchHandlernow short-circuits when the matcher seteventMatchedon the current action: it advances down the matched branch and clears the flag, instead of re-running the property condition.workflows-wait-until-eventfeature flag so it can be rolled out to a single team first.(woken by event: X)when the matcher woke the job (readingstate.currentAction.eventMatchedEventpopulated by the matcher), so the log distinguishes the wake event from the trigger event instead of echoing the trigger event for both.Wait until condition→Wait untilandWait until window→Time windowso the names reflect what the steps actually do now.This is stacked on #57847 (it depends on the matcher consumer, the
eventMatched/eventMatchedEventstate fields, and theeventsschema field). Base will retarget tomasteronce #57847 merges.How did you test this code?
ConditionalBranchHandlerunit tests for theeventMatchedshort-circuit: advancing to the matched branch + clearing the flag, and falling through to condition evaluation when unset.workflows-e2e.test.tsexercising the real matcher consumer + cyclotron worker againsttest_cyclotron_node: event-driven wake → matched branch, property-condition wake → matched branch, timeout → continue branch, and unrelated event leaves the job parked.conditional_branch, matcher consumer, andworkflows-e2eJest suites and the nodejs typecheck locally.Wait untilstep configured with an "Events to wait for" entry behind the feature flag), fired the trigger event, confirmed the job parked, fired the wake event, watched the matcher wake the job, the executor's resume log surface(woken by event: …), the handler advance down the matched branch, and the downstream Email action fire to completion.Publish to changelog?
no
🤖 Agent context
Authored with Claude Code. The handler and UI were originally prototyped on a combined branch; this PR carries them on top of the matcher PR with a feature-flag gate, handler test coverage, the executor wake-event log surface, the two side-panel renames, and a set of end-to-end scenarios in
workflows-e2e.test.ts. Handler + UI are bundled in one PR because each is inert without the other (the UI produces config the handler consumes), and the handler change is a no-op for any team that has not configured event waits.