-
Notifications
You must be signed in to change notification settings - Fork 2k
Automation Looping v2 #17055
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
base: master
Are you sure you want to change the base?
Automation Looping v2 #17055
Conversation
…to feat/looping-v2-frontend
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
...ges/builder/src/components/automation/AutomationBuilder/FlowChart/SelectStepSidePanel.svelte
Show resolved
Hide resolved
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
...ges/builder/src/components/automation/AutomationBuilder/FlowChart/SelectStepSidePanel.svelte
Show resolved
Hide resolved
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.
To be honest this is beyond my attention span to review! With that said there is nothing in particular I could see wrong with the code.
I'll pull the branch down locally and give it a test and then come back and approve.
packages/builder/src/components/automation/AutomationBuilder/FlowChart/AutomationStepHelpers.ts
Outdated
Show resolved
Hide resolved
? { x: node.position.x, y: node.position.y + delta } | ||
: { x: node.position.x + delta, y: node.position.y } | ||
const nexts = outgoing[id] || [] | ||
for (const nId of nexts) stack.push(nId) |
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.
NAB: I wonder if you can just do a map instead on nexts ?
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.
Why? I think this is pretty clean
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.
Yeah it's fine, but could be a one liner instead of a collapsed two liner:
stack.push(...nexts)
…assumed to be zero and better accommodate large loop blocks when centering automations
Description
Implements the frontend for Automation Looping v2:
Screenshots
Launchcontrol
Feature branch env
Feature Branch Link