feat: Add UI to actions whose definitions indicate they return a result, to optionally store that result in a local or custom variable#4065
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds support for capturing and storing action return values in Companion variables. It refactors internal action result handling from boolean to structured types, introduces a generalized special-expression manager to track computed metadata (isInverted, storeResult), creates LocalVariablesController for local variable management, and updates action execution paths across process boundaries to propagate result values through storage targets. ChangesAction Result Storage & Special Expression Framework
Action Execution & Result Capture
Cross-Process Communication & IPC
Tests
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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
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 |
|
I'm wondering if this should work such that for any action definition which says 'i can return a value' we let the user pick a local-variable name to write the value into. This would make it similar to how the generic-http module (and others?) do this today, but as they predate local-variables they can only write to custom variables. Perhaps it should be able to do both, but lets limit this to local to begin with. I see pros and cons to each:
I'm not opposed to something like this in the future, but I think what we use as the identifier needs some thought first. Something like this would be a start to addressing some other requests to reference/update properties of actions/feedbacks (some of which I am not sure is a good idea) |
jswalden
force-pushed
the
action-callback-return-result
branch
from
21b0e38 to
e646622
Compare
My concern here is that an action that sets a result, should not have that result remain live for unbounded period of time. (At least not unless a subsequent user of it chooses to keep it live.) Tying results directly and only to a bounded set of sequentially-next actions does this. Whereas if you could refer backwards to previous actions, perhaps even dynamically, then every action's result has to be kept alive "just in case". There are plainly a bunch of ways to skin this cat. "Use it immediately or lose it" doesn't seem like too complicated a way to grasp, to me. |
That makes some idiomatic sense.
Yeah. (If I were to grump on terminology, I'd name local variables as "per-entity variables", or better non-internal idiom terminology. And then maybe have some kind of truly local variable that can be created by an internal action, set from expression [or with this enhancement from an action result], that remains live only for the duration of the sequential action group it's found in. And would not have concurrency considerations to think about. But I digress.) I guess the concurrency of local variables is my biggest quirk/beef here. Same as, but at least significantly reduced in troublesomeness, of a module variable, or of writing to a custom variable.
I think you could arguably say that implicitly piping the result distributing it across a concurrent action group is...of uncertain intuitiveness, as mechanism to enable multiple distinct manners of consuming an action result. So possibly the root problem isn't really the waits. (I don't fully understand why waits in concurrent action groups are even given special meaning. Why not just require users to create a sequential action group, then interleave waits between concurrent action groups? But I digress even harder...)
Forcing stuff to be named doesn't feel like boilerplate to me. What does feel like boilerplate, is having to create/name them out of the actual UI of the step flow being executed. And having that variable exist across all steps, rather than only narrowly in the exact set of actions the computation/initialization happens in. |
it was done to make it easier to program these buttons. having a wait inside of a concurrent group otherwise makes no real sense, and this way we probably cut down the number of groups the user needs to add massively (perhaps even avoiding them altogether). The thing to remember is that this was to replace the old behaviour where we never awaited actions, and it was possible to give each action an either relative or absolute start delay. That relative mode is what this is emulating. And consider that adding a delay used to be simply typing in a number. Then it moved to an action and many users couldnt find it. Making them also add a group and drag their actions around to arrange them to get the same behaviour would likely not have been well received.
Another solution to concurrency issues would be to add a checkbox onto the 'user value' type, so that it ignores external updates while running the stack of actions.
True, I didn't consider that. It for sure wont be able to work within the concurrent group, but the group could return a single value (from I guess the last?) to be used later? But this will make it hard to explain to users what the variable will refer to I think my main concern here is just that I feel this will be a half-solution to concurrency issues over using local-variables. I expect that in many cases users will still want or need to write the result to a local-variable (even if just to make debugging/tracing the values easier). For anything sensitive to concurrency, I do also wonder if our behaviour of allowing it to run in parallel is good enough. Not an argument against this, just that if we adopt a flow like home-assistant where they have modes: So I am leaning towards saying that I think this needs to be done with local-variables, as this approach essentially does not work in concurrent groups. For 4.3, I am open to including the ability to write action results to local-variables (but be aware that I had originally intended to release 4.3 by now, so that could happen any day now). And any work on better handling concurrency I would like to delay for 4.4, so it can have some time to be tested in the betas. Doesn't mean it can't be implemented now, it just won't be merged until 4.3 is finished |
It's entirely reasonable for a concurrent group's result to always be (I need to tweak slightly so an empty sequential group results in
It mostly is a Companion-side decision how/where to sink result values, once the base/host side of it is adequately solved. Sink into something can be done as first step. Enabling sink into more stuff is deferrable.
Given the name "concurrent", nobody should expect bare act of yielding a result to be usable in them. Get a value, then set it somewhere, by nature incorporates a dependency that requires sequential operation.
...short of there being some kind of breaking API change to make, and wanting it to be in before the 2.0 API actually ships, I don't think you should delay. I've been on the side of things you might want in a release, that nevertheless were rejected for one because not in time for it. Logrolling near end of a cycle can end badly, and always creates precedent for more delays in the future. It's rarely worth it for a bare enhancement. If this is not in time, so be it. There's always next time. That said. If this is a time where breaking API changes can be made still -- is it? is a hypothetical 2.0.1 or most pedantically 3.0.0 out of line now, before there has been a Companion release supporting it? -- if you changed the callback return type to
I don't think an action that returns a result can ever be meaningfully usable (at least as to the result value; side effects are still usable, of course) inside a concurrent group. And I will reiterate that I don't think you should hold anything up for this -- save that if we are still at a juncture where incompatibly changing the type that |
jswalden
force-pushed
the
action-callback-return-result
branch
2 times, most recently
from
b4c8e2d to
8bc3b8d
Compare
jswalden
changed the title
$(this:result)
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36662a95-8712-48ad-9268-a1d9a29fe0d9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
companion/lib/Controls/ActionRunner.tscompanion/lib/Instance/Connection/ChildHandlerApi.tscompanion/lib/Instance/Connection/ChildHandlerLegacy.tscompanion/lib/Instance/Connection/ChildHandlerNew.tscompanion/lib/Instance/Connection/IpcTypesNew.tscompanion/lib/Instance/Connection/Thread/Entrypoint.tscompanion/lib/Internal/Controller.tscompanion/lib/Internal/Variables.tscompanion/lib/Variables/Values.tscompanion/package.json
✅ Files skipped from review due to trivial changes (2)
- companion/lib/Internal/Controller.ts
- companion/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- companion/lib/Instance/Connection/ChildHandlerApi.ts
- companion/lib/Variables/Values.ts
- companion/lib/Instance/Connection/Thread/Entrypoint.ts
- companion/lib/Instance/Connection/ChildHandlerNew.ts
jswalden
force-pushed
the
action-callback-return-result
branch
from
8bc3b8d to
4fe31cf
Compare
|
Updated to add actions to set a local/custom variable from the result of a series of actions. A series of actions is allowed mostly because I cribbed from |
jswalden
force-pushed
the
action-callback-return-result
branch
from
4fe31cf to
07b8e3a
Compare
|
There's already I think that's everything previously mooted here, so this seems ready to be taken for this release, or sat on til after release. I'd still argue for the mini-type-fix of making callbacks return |
|
I'm not keen on this requiring putting an action inside another action to be able to save the result. I am worried about a combination of:
I would much rather that this added an extra field in the ui to any actions which support this, handled within companion. I think the closest we have to this today is the inverted field for boolean feedbacks. So I am wondering about a couple of auto-added fields at the bottom of the action by the ui, when the action supports this:
I think that by doing this we both solve the discovery issue with minimal ui bloat, matching a flow users are already used to (in generic-http and friends), and simplify usage by making it a couple of clicks to setup. In either case the ui will need to be aware of which actions support this, so that it can stop the user adding an action to the For the implementation of this, I wonder if we should have an 'internalOptions' object on each entity, for storing these values? This could be well typed too I expect. Ideally the feedback 'inverted' property could be moved into this too, but I would leave that for a follow up as that will be a bit fiddly to untangle. As a follow up to this (for me to do), I will look into what we can do to auto-migrate some existing modules across to this instead of using the old |
This seems like solidly better UI than my approach.
The
On no more than two minutes' thought that seems plausible. Need to noodle a little more, not at EOD, more before being certain. I'll try to get back on this in the next few days. |
yes it should be. defining that requirement in the generic-ness of the entity child lists requires a little bit more glue to handle. but hopefully not necessary now
no rush :) |
|
Just piping in here, based on my experience working with the value feedbacks, it would make life considerable easier for users if the current value or last value (if that makes more conceptual sense here) of the returned variable was brought to the UI along with a copy button for the variable name. Before these were added in 4.3 I was frequently having to drop local variables on buttons just to confirm values while configuring an action/feedback. |
jswalden
force-pushed
the
action-callback-return-result
branch
from
07b8e3a to
52081a2
Compare
|
Okay, for the most part this is all "ready" to go. At least in terms of it not being a waste to look at, if nothing else. The extent of change has grown a fair bit from the "just add some actions that can do all the stuff" previous state. The final two commits here are not for committing, ignore them and/or realize they would be excluded. That done, we have:
This is the bit about local-variable manipulation only being possible through the variables fragment now, and this creating a controller that
In order for internal actions to be able to return results -- none do now, but I feel like I ought be making it possible for them to, so when you eventually want such an action you don't have to do the plumbing yourself -- I change
This just gets the internal action
What this PR aims to do, is allow for another field similar to It seems to me that the simplest/clearest way to do this, is to generalize is-inverted handling so that it's just one instance of "special expression", that exists outside of option values. So in this rev I do that -- defining a record of special expression name to expression value, which is initially just Updating is still debounced acros all updates. Entities are tracked by the specific special expression at issue. Entities are forgotten across all special expressions at once -- but it would be easy to add a later optimization to forget only for specific cases.
I didn't rename any files for the new class names in the previous rev. This renames them, separately so as not to break readable rev history.
This is the biggest meat of change. There is not a lot you could do to trim it down. I guess the webui changes could have been split into their own rev, and the actionrunner changes could be a separate rev, and and and. I tried to simply copy pretty heavily the I added no tests for any of this, just worked through the UI manually. I can likely be guilted into adding tests, but this probably needs a skim before I discover I've gone astray somewhere that would invalidate any test-writing. The names for "storeActionResultTarget", "storeResultTarget", etc. are under-thought-out and mostly just named to have a name to refer to at all. I did not have ideas I really liked on this. Maybe I don't really understand the logic of Most of the scattered I assume the This PR is not developed against My React skills are meager, and I almost can't believe my Oh, and the final patch that actually makes webui changes, that depends on #217 landing and an updated I think that's a summary of everything here that is worth flagging. There are things I could do to split up that final patch (e.g. move the action option-commoning into shared-lib into a separate rev), but those aren't really the parts of it that are questionable. |
jswalden
force-pushed
the
action-callback-return-result
branch
from
52081a2 to
1bfc4dc
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/test/Controls/Entities/EntitySpecialExpressionManager.test.ts (1)
1-679:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlease run Prettier on this test file before merge.
CI is already failing format checks for this file, so this will block merge until fixed.
companion/lib/Controls/Entities/EntityInstance.ts (1)
691-706:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
replacePropsdoesn’t propagate actionstoreResult, so replacements can keep stale target state.With the new store-result feature, replacing an action should also update the underlying action model’s
storeResult.Suggested fix
replaceProps(newProps: SomeReplaceableEntityModel, skipNotifyModule = false): void { this.#data.definitionId = newProps.definitionId this.#data.options = newProps.options this.#data.upgradeIndex = newProps.upgradeIndex if (this.#data.type === EntityModelType.Feedback) { const feedbackData = this.#data as FeedbackEntityModel const newPropsData = newProps as FeedbackEntityModel feedbackData.isInverted = newPropsData.isInverted ?? feedbackData.isInverted @@ : feedbackData.styleOverrides + } else if (this.#data.type === EntityModelType.Action) { + const actionData = this.#data as ActionEntityModel + const newActionProps = newProps as ActionEntityModel + actionData.storeResult = newActionProps.storeResult }
🧹 Nitpick comments (2)
companion/lib/Variables/LocalVariablesController.ts (1)
38-54: 💤 Low valueConsider adding a safety check for entity type.
The
#getControlAndVariablehelper assumes that any entity with a matchingrawLocalVariableNameis the correct target. While theisInternalUserValueFeedbackcheck on line 51 provides type safety, it might be clearer to filter for the expected entity type earlier in the lookup chain (line 45) to avoid potential confusion if multiple entity types could theoretically have the samerawLocalVariableName.That said, if the current architecture guarantees unique
rawLocalVariableNamevalues across all entity types on a control, this is fine as-is! Just wanted to flag it in case there are edge cases I'm not aware of.companion/lib/Controls/Entities/EntityInstance.ts (1)
329-334: ⚡ Quick winConsider tracking
isInvertedonly for feedback entities.Right now actions are also queued for
isInvertedprocessing, which is unnecessary work and noise in update batches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: adc205f2-d82e-4dc9-bc27-33bae41e2705
📒 Files selected for processing (34)
companion/lib/Controls/ActionRunner.tscompanion/lib/Controls/Entities/EntityInstance.tscompanion/lib/Controls/Entities/EntityIsInvertedManager.tscompanion/lib/Controls/Entities/EntityList.tscompanion/lib/Controls/Entities/EntityListPoolBase.tscompanion/lib/Controls/Entities/EntityListPoolButton.tscompanion/lib/Controls/Entities/EntityListPoolExpressionVariable.tscompanion/lib/Controls/Entities/EntityListPoolTrigger.tscompanion/lib/Controls/Entities/EntitySpecialExpressionManager.tscompanion/lib/Controls/Entities/SpecialExpressions.tscompanion/lib/Controls/Entities/Types.tscompanion/lib/Controls/EntitiesTrpcRouter.tscompanion/lib/Instance/Connection/ChildHandlerApi.tscompanion/lib/Instance/Connection/ChildHandlerLegacy.tscompanion/lib/Instance/Connection/ChildHandlerNew.tscompanion/lib/Instance/Connection/IpcTypesNew.tscompanion/lib/Instance/Connection/Thread/Entrypoint.tscompanion/lib/Instance/Connection/Thread/HostContext.tscompanion/lib/Internal/ActionRecorder.tscompanion/lib/Internal/BuildingBlocks.tscompanion/lib/Internal/Controller.tscompanion/lib/Internal/Controls.tscompanion/lib/Internal/CustomVariables.tscompanion/lib/Internal/Instance.tscompanion/lib/Internal/Surface.tscompanion/lib/Internal/System.tscompanion/lib/Internal/Triggers.tscompanion/lib/Internal/Types.tscompanion/lib/Internal/Util.tscompanion/lib/Internal/Variables.tscompanion/lib/Registry.tscompanion/lib/Variables/LocalVariablesController.tscompanion/test/Controls/Entities/EntityList.test.tscompanion/test/Controls/Entities/EntitySpecialExpressionManager.test.ts
💤 Files with no reviewable changes (2)
- companion/lib/Internal/Util.ts
- companion/lib/Controls/Entities/EntityIsInvertedManager.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- companion/lib/Instance/Connection/ChildHandlerApi.ts
- companion/lib/Instance/Connection/ChildHandlerLegacy.ts
jswalden
force-pushed
the
action-callback-return-result
branch
2 times, most recently
from
8ff7b69 to
1aa7033
Compare
jswalden
changed the title
Julusian
left a comment
Julusian
left a comment
There was a problem hiding this comment.
Sorry for taking so long to properly look at this.
It looks good!
I have a handful of small niggles, but nothing major
|
@jswalden I have updated the module-api after merging your pr there, so there are a couple of small merge conflicts here now. This does mean that once you have resolved it the tests and types should be fully happy though |
…iables` module fragment into a standalone controller.
…an be used in `webui/` in a future change.
…t can be used in `webui/` in a future change.
…into `shared-lib/` for future use in `webui/`.
…switch` statements instead of `if`/else if`/`else`.
…at is for the moment ignored).
…ions return results.
…he moment simply discard it.
jswalden
force-pushed
the
action-callback-return-result
branch
from
1aa7033 to
d2b50d1
Compare
|
Okay, responded to all review comments...plus a touch more. Running through the notable adjustments: New special expression values can be computed in standalone functionsAfter the generalize-
for the reason in the summary. I did this because it seemed like the change ought be in the initial refactor, not tacked atop handling for Don't track
|
…de into a system that allows additional kinds of special expressions to be handled.
…esult in a local or custom variable. bitfocus#4065
Julusian
force-pushed
the
action-callback-return-result
branch
from
cc0f570 to
e4d9367
Compare
4 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
Release Notes
New Features
Bug Fixes