feat: Allow actions to return result values, exposed to the subsequent sequential action as $(this:result)#4065
Conversation
…entially succeeding actions as `$(this:result)`.
📝 WalkthroughWalkthroughThis pull request introduces action result chaining, allowing actions executed sequentially to access the previous action's result via a new Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
companion/lib/Internal/Controller.ts (1)
448-470:⚠️ Potential issue | 🟠 MajorInternal action results are still dropped on this path.
This now wires
$(this:result)into parsing, but Line 448 still returnsPromise<void>, and the fragment dispatch below still treats the fragment return as a truthy “handled” flag. The result is that internal actions cannot propagate any value to the next action, and falsy results like0,false, or''additionally fall through as unhandled. Please switch this path to an explicit handled/result contract and return the actual result fromexecuteAction().Also applies to: 496-505
companion/lib/Controls/ActionRunner.ts (1)
89-110:⚠️ Potential issue | 🟠 MajorPlease lock down
previousResultsemantics across concurrent groups with waitsFriendly heads-up: actions after a wait currently still receive the same
extras.previousResultas actions before the wait (Line 104), which matches the prototype but leaves ambiguous behavior in a user-visible path. This can make$(this:result)feel non-deterministic once waits are involved.I’d recommend choosing and enforcing one explicit policy here (e.g., clear after first wait, or disable propagation when any wait exists).
Suggested direction (disable propagation when waits are present)
} else { const groupedActions = this.#splitActionsAroundWaits(actions) + const hasWaits = groupedActions.some((group) => !!group.waitAction) const ps: Promise<VariableValue>[] = [] for (const { waitAction, actions } of groupedActions) { if (extras.abortDelayed.aborted) break @@ // Spawn all the actions in parallel for (const action of actions) { + const actionExtras: RunActionExtras = { + ...extras, + previousResult: hasWaits ? undefined : extras.previousResult, + } ps.push( - this.#runAction(action, extras).catch((e) => { + this.#runAction(action, actionExtras).catch((e) => { this.#logger.silly(`Error executing action for ${action.connectionId}: ${e.message ?? e}`) return undefined }) ) }
🧹 Nitpick comments (2)
companion/lib/Controls/IControlStore.ts (1)
48-52: Consider makingpreviousResultoptional in the interfaceNice addition overall. For this interface, making
previousResultoptional can keep behavior the same while avoiding repetitiveundefinedat every non-action call site.Proposed tweak
createVariablesAndExpressionParser( controlId: string | null | undefined, overrideVariableValues: VariableValues | null, - previousResult: VariableValue + previousResult?: VariableValue ): VariablesAndExpressionParsercompanion/lib/Controls/ActionRunner.ts (1)
75-83: Nice chaining logic — consider avoiding in-place mutation ofextrasThis works, but mutating
extras.previousResultdirectly (Line 77) makes the function stateful against its input object. Using a localpreviousResult(and passing a fresh extras object per action) keeps behavior easier to reason about.Refactor sketch
if (executeSequential) { // Future: abort on error? + let previousResult = extras.previousResult for (const action of actions) { if (extras.abortDelayed.aborted) break - extras.previousResult = await this.#runAction(action, extras).catch((e) => { + previousResult = await this.#runAction(action, { ...extras, previousResult }).catch((e) => { this.#logger.silly(`Error executing action for ${action.connectionId}: ${e.message ?? e}`) return undefined }) } - return extras.previousResult + return previousResult
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d3bd3af-aba0-4f33-b03d-b6cd4ddcdc72
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
companion/lib/Controls/ActionRunner.tscompanion/lib/Controls/ControlStore.tscompanion/lib/Controls/ControlTypes/Button/Base.tscompanion/lib/Controls/ControlTypes/Button/Preset.tscompanion/lib/Controls/ControlTypes/Button/Util.tscompanion/lib/Controls/ControlTypes/Triggers/Trigger.tscompanion/lib/Controls/Controller.tscompanion/lib/Controls/Entities/EntityIsInvertedManager.tscompanion/lib/Controls/Entities/EntityListPoolBase.tscompanion/lib/Controls/IControlStore.tscompanion/lib/ImportExport/Backups.tscompanion/lib/ImportExport/Export.tscompanion/lib/Instance/Connection/ChildHandlerApi.tscompanion/lib/Instance/Connection/ChildHandlerLegacy.tscompanion/lib/Instance/Connection/ChildHandlerNew.tscompanion/lib/Instance/Connection/EntityManager.tscompanion/lib/Instance/Connection/IpcTypesNew.tscompanion/lib/Instance/Connection/Thread/Entrypoint.tscompanion/lib/Internal/Controller.tscompanion/lib/Preview/ExpressionStream.tscompanion/lib/Preview/Graphics.tscompanion/lib/Surface/Controller.tscompanion/lib/Variables/Values.tscompanion/package.jsondocs/user-guide/3_config/variables.mdwebui/src/Controls/LocalVariablesStore.tsx
| - The result of the action sequentially preceding this one | ||
| - Variable: `this:result` |
There was a problem hiding this comment.
Please add a short caveat for concurrent/wait action groups
Love that this:result is documented. Given current implementation notes, a brief line clarifying that behavior is guaranteed for sequential chains (and may differ for concurrent groups with waits) would help avoid user confusion.
There was a problem hiding this comment.
Brevity/clarity versus precision tradeoff. Can tweak if someone wants it.
There was a problem hiding this comment.
@jswalden — totally fair point! Here's a minimal phrasing that tries to stay concise while flagging the edge case:
- The result of the action sequentially preceding this one (sequential action chains only)
- Variable: `this:result`Or alternatively, a single parenthetical note tucked on the same line keeps the list clean without adding extra bullet depth. Either way, happy to leave it as-is if you'd prefer to revisit once the concurrent-group propagation semantics are finalized — the current wording isn't wrong, just silent on the edge case.
I chose for internal actions to not produce their own results, and also not to propagate a prior result across them. (Why should they? Don't see good reason for them to, honestly.) It makes most sense IMO for only an action that produces a result itself, to be source of one.
Flagged in the initial comment.
Explicit is better than implicit. There's always a previous result value/variable. It's not actually optional semantically. (I even considered using
I kind of prefer less object creation churn, but Julian's the one who will have a meaningful preference here, probably. |
|
This will fix #4064 |
jswalden
changed the title
$(this:result). (#4064)$(this:result)
|
The idea is awesome and I 100% support it but my suggestion to your question
Why not use something like n8n does it: any node that's downstream, can access any upstream node's result using the node's name. For example, first node is called "Message", the next node can get the data directly with |
2 participants
The one moderately-major thing in all this that I am uncertain about, is how to propagate a result to a subsequent concurrent action group that contains waits.
If I read the code correctly, Companion basically splits concurrent action groups into sets of non-wait actions, and then it performs all those non-waits of the first set at once, then does the intervening wait, then does the next set of non-waits, etc.
I think it is reasonable to propagate a previous result to all actions in a concurrent action group that doesn't contain any waits. It is less clear that it makes sense to do so when the action group contains waits. Or that they should propagate beyond the first set of actions? For prototype-ful purposes I just acted like the waits weren't there. For actual suitability for landing purposes, maybe that should change. Don't propagate if there are any waits? Only propagate to actions before the first wait? The answer isn't immediately obvious to me.
Summary by CodeRabbit
New Features
$(this:result)variable, allowing subsequent actions to dynamically access and use the output of the preceding actionDocumentation
$(this:result)builtin local variable, providing access to the result returned by the previously executed action in an action sequence