feat: support expressions for most internal action fields #2345 #3617
Conversation
Julusian
changed the title
Does that mean all those options actually support it, or only the last one? What's the use case for entering a surface/group name by expression (as opposed to it's ID)? FWIW I think to actually solve #3235 it needs some new actions. |
in the screenshot, all of them do with the last one currently being set to expression mode.
Or maybe it just needs a variable listing all the surfaces? Its already possible to create loops #3619
I dont have one, but Im sure someone will. we already support taking it from a variable (using a few fields and isVisible to do so), so changing it to expressions is largely to simplify and unify it. |
|
@dnmeid any thoughts on the approach taken here? (how the values are stored, and how the ui works) |
Julusian
force-pushed
the
feat/expressions-internal-actions
branch
from
56541da to
052a950
Compare
📝 WalkthroughWalkthroughRefactors internal execution to use options-based action/feedback types and adds expression-aware option handling, parsers, and utilities; removes InternalModuleUtils from constructors and updates many module signatures and client entity shapes to include expression metadata. 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webui/src/Controls/OptionsInputField.tsx (1)
14-80: Handle legacy unwrapped values when expression support is on.
rawValueis cast toExpressionOrValuewithout a runtime guard. If old data still stores primitives,valuebecomesundefinedand the control renders blank. It’d be safer to normalize withisExpressionOrValueand fall back to{ isExpression:false, value: rawValue }.🔧 Suggested fix
-import type { ExpressionOrValue, SomeCompanionInputField } from '@companion-app/shared/Model/Options.js' +import { isExpressionOrValue, type ExpressionOrValue, type SomeCompanionInputField } from '@companion-app/shared/Model/Options.js' @@ - const value = fieldsSupportExpressions ? (rawValue as ExpressionOrValue<any>)?.value : rawValue + const normalizedExpressionValue = fieldsSupportExpressions + ? isExpressionOrValue(rawValue) + ? rawValue + : { isExpression: false, value: rawValue } + : undefined + + const value = fieldsSupportExpressions ? normalizedExpressionValue?.value : rawValue @@ - const rawExpressionValue = (rawValue as ExpressionOrValue<any>) || { isExpression: false, value: undefined } + const rawExpressionValue = normalizedExpressionValue || { isExpression: false, value: undefined }Also applies to: 213-219
companion/lib/Internal/Surface.ts (1)
126-134: Guard against nullish/emptysurfaceIdresults from expressions.
String(undefined)becomes"undefined", which can lead to attempts to control a phantom surface instead of short‑circuiting.✅ Suggested fix
- let surfaceId: string | undefined = String(options.surfaceId).trim() + if (options.surfaceId === undefined || options.surfaceId === null) return undefined + let surfaceId = String(options.surfaceId).trim() + if (!surfaceId) return undefined
🧹 Nitpick comments (3)
webui/src/scss/_common.scss (1)
352-369: Small layout bug:justify-selfwon’t work in flex here.Since
.field-with-expressionisdisplay: flex,justify-selfon.expression-toggle-buttonis ignored. Usemargin-left: autooralign-selfinstead so the toggle actually aligns to the end.💡 Suggested tweak
.field-with-expression { display: flex; gap: 0.5rem; .expression-field { flex-grow: 1; } .expression-toggle-button { - justify-self: flex-end; + margin-left: auto; .btn { height: 100%; width: 3em; } } }docs/user-guide/3_config/variables.md (1)
116-117: Nice addition! Consider clarifying the format.Thanks for documenting the new
this:locationvariable! The entry looks great and follows the existing pattern perfectly.One small suggestion: it might be helpful to briefly mention what format the location value takes (e.g., is it something like
"1/2/3", an object, or another format?). This would help users understand what they'll get when they use it in their expressions or actions.companion/lib/Internal/Controls.ts (1)
746-764: Good catch on recursively upgrading child actions!The recursive
this.actionUpgrade(newChildAction, controlId)call (line 748) ensures newly created child actions get fully upgraded.The TODO on line 763 about recursive feedback upgrades is worth tracking. Would you like me to open an issue for this, or is it something you're planning to address in this PR?
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 (1)
companion/lib/Internal/Controls.ts (1)
747-826: Potential gap:bank_current_stepaction may keep legacystepvalues unwrapped.
Whenstepalready exists (old literal), the upgrade path skips wrapping it into{ isExpression, value }. That can leave mixed option shapes and confuse expression toggling or parsing.✅ Suggested fix
} else if (action.definitionId === 'bank_current_step') { changed = convertOldLocationToExpressionOrValue(action.options) || changed if (action.options.step === undefined) { convertOldSplitOptionToExpression( action.options, { useVariables: 'step_from_expression', variable: 'step_expression', simple: 'step', result: 'step', }, true ) changed = true } + changed = convertSimplePropertyToExpressionValue(action.options, 'step') || changed }Also noticed the TODO about recursively upgrading the new feedback—if you still want that, I’m happy to help sketch a follow-up.
♻️ Duplicate comments (2)
webui/src/Components/FieldOrExpression.tsx (1)
42-50: Consider coercing to string when switching to expression mode.Just a friendly heads-up - when toggling to expression mode from a non-string field (like a number or color picker),
value.valuemight not be a string. TheExpressionOrValue<T>type expectsvalue: stringwhenisExpression: true.While
stringifyVariableValuehandles the conversion at render time (line 63), the stored state could be inconsistent with the type contract. You might want to coerce when switching modes:💡 Suggested approach
const setIsExpression = useCallback( (isExpression: boolean) => { setValue({ isExpression, - value: value.value, + value: isExpression ? (stringifyVariableValue(value.value) ?? '') : value.value, }) }, [setValue, value] )companion/lib/Internal/Variables.ts (1)
348-367: Pipeline blocker:nocommitcomment needs to be resolved.Hey! The CI is failing because of the
nocommitcomment on line 350. It looks like there's also some commented-out code below that could be cleaned up.I understand this is WIP, but to get CI green you'll need to either:
- Address the TODO and remove the
nocommitmarker- Convert it to a regular
TODOcomment if it's intentional WIP that can be mergedThe commented-out block (lines 354-366) also looks like remnants from the old implementation - feel free to remove that when you clean this up!
🔧 Suggested cleanup
- // nocommit - this needs to be adjusted to handle triggers/expression variables + // TODO - this needs to be adjusted to handle triggers/expression variables const location = ParseLocationString(String(action.options.location), extras.location) const theControlId = location ? this.#pageStore.getControlIdAt(location) : null - - // let theControlId: string | null = null - // if (action.rawOptions.location_target === 'this') { - // // This could be any type of control (button, trigger, etc) - // theControlId = extras.controlId - // } else { - // // Parse the location of a button - // const result = this.#internalUtils.parseInternalControlReferenceForActionOrFeedback( - // extras, - // action.rawOptions, - // true - // ) - // theControlId = result.location ? this.#pageStore.getControlIdAt(result.location) : null - // }
Julusian
force-pushed
the
feat/expressions-internal-actions
branch
from
4ba9ea5 to
c92aaae
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/lib/Variables/CustomVariable.ts (1)
205-216: AligncreateVariablevalidation with the widenedVariableValuesignature.Right now the signature accepts
VariableValue | undefined, but runtime rejects anything non-string. That mismatch will surprise callers and makes the signature misleading. Either revert the signature back tostringor broaden the runtime guard (and TRPC input) to the allowedVariableValueunion.💡 Possible adjustment (align guard with allowed VariableValue types)
- if (typeof defaultVal !== 'string') { - return 'Bad default value' - } + if ( + defaultVal !== undefined && + typeof defaultVal !== 'string' && + typeof defaultVal !== 'number' && + typeof defaultVal !== 'boolean' + ) { + return 'Bad default value' + }
♻️ Duplicate comments (5)
companion/lib/Resources/Visitors/ReferencesCollector.ts (1)
101-106: Skip expression-mode values when collecting references.
#getAndUnwrapPropertyValuereturns.valueeven whenisExpressionis true, so expression strings get treated as literal IDs/variables. That can silently add incorrect references.🔧 Suggested fix
`#getAndUnwrapPropertyValue`(obj: Record<string, any>, propName: string): any { const value = obj[propName] - if (isExpressionOrValue(value)) { - return value.value - } + if (isExpressionOrValue(value)) { + return value.isExpression ? undefined : value.value + } return value }webui/src/Components/FieldOrExpression.tsx (2)
42-47: Coerce to string when switching into expression mode.When toggling to expression mode,
value.valuecan still be a number/boolean, which may break later parsing or validation. Coerce to string at the toggle boundary.🔧 Suggested fix
const setIsExpression = useCallback( (isExpression: boolean) => { setValue({ isExpression, - value: value.value, + value: isExpression ? String(value.value ?? '') : value.value, }) }, [setValue, value] )
75-75: Tiny UI polish: remove the trailing space in the title.✏️ Quick fix
- title={value.isExpression ? 'Expression mode ' : 'Value mode'} + title={value.isExpression ? 'Expression mode' : 'Value mode'}companion/lib/Internal/System.ts (1)
329-334: Upgrade should convertmessage, notcustom_log.This still targets the wrong option id, so legacy
custom_logactions won’t migrate to the expression/value shape.💡 Suggested fix
- if (action.definitionId === 'custom_log') { - changed = convertSimplePropertyToExpressionValue(action.options, 'custom_log') || changed + if (action.definitionId === 'custom_log') { + changed = convertSimplePropertyToExpressionValue(action.options, 'message') || changed } else if (action.definitionId === 'exec') {companion/lib/Internal/Variables.ts (1)
350-366: Heads up:nocommitcomment will fail CI.The ESLint
no-warning-commentsrule is catching thenocommitcomment on line 350. The commented-out code block (lines 354-366) also looks like remnants from the old implementation that should be cleaned up.I see this is still WIP for handling triggers/expression variables. When you're ready to finalize, you could either:
- Address the TODO and remove the comment, or
- Convert it to a regular
TODO(without "nocommit") if it's intentional WIP that shouldn't block the PRNo rush - just flagging so it doesn't surprise you in CI! 😊
🧹 Nitpick comments (4)
webui/src/scss/_common.scss (1)
353-369: Nice addition for the expression toggle UI! 👍The layout structure looks good and will work as intended. One small note: on line 362,
justify-self: flex-enddoesn't actually do anything in a Flexbox context—it's a Grid property. The button ends up on the right side anyway because.expression-fieldhasflex-grow: 1, so everything still works fine!If you'd like to tidy this up, you could simply remove that line since it's not contributing to the layout. But totally optional—no functional impact either way.
💅 Optional cleanup
.expression-toggle-button { - justify-self: flex-end; - .btn { height: 100%; width: 3em; } }docs/user-guide/3_config/variables.md (1)
116-117: Nice addition! Consider clarifying the location format.The documentation follows the established pattern perfectly and is a welcome addition to the builtin local variables section.
Since "location" is likely a composite value (unlike the simple numeric values for page, column, and row), it might be helpful to clarify what format this variable contains. For example, does it return something like
"1/2/3"or"page:1,row:2,col:3"or another format? Adding a brief note about the format or a small example would help users understand what to expect when they use this variable.📝 Optional enhancement suggestion
Consider adding a brief format description, for example:
- Location of the button - - Variable: `this:location` + - Variable: `this:location` (format: `page/row/column`)Or add an example if that would be clearer for users.
companion/lib/Variables/VariablesAndExpressionParser.ts (1)
169-171: Type assertion toas anynoted.The
as anycast at line 170 is understandable given the flexibleExpressionOrValue<JsonValue>typing, but worth keeping an eye on. If the type system evolves, this could be tightened up later.companion/lib/Internal/Surface.ts (1)
349-360: Duplicate condition check infeedbackUpgrade.Lines 352-354 and 355-357 both check
feedback.definitionId === 'surface_on_page'. These could be combined into a single block:♻️ Suggested consolidation
feedbackUpgrade(feedback: FeedbackEntityModel, _controlId: string): FeedbackEntityModel | void { let changed = false if (feedback.definitionId === 'surface_on_page') { changed = convertSimplePropertyToExpressionValue(feedback.options, 'surfaceId', 'controller', 'self') || changed - } - if (feedback.definitionId === 'surface_on_page') { changed = convertSimplePropertyToExpressionValue(feedback.options, 'page') || changed } if (changed) return feedback }
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 (1)
companion/lib/Internal/Surface.ts (1)
126-135: Guard againstString(undefined)in#fetchSurfaceId.If
surfaceIdis missing,String(undefined)becomes"undefined"and bypasses the!surfaceIdchecks later. Consider a nullish guard before coercion (Line 130).💡 Suggested fix
- let surfaceId: string | undefined = String(options.surfaceId).trim() + const rawSurfaceId = options.surfaceId + if (rawSurfaceId === undefined || rawSurfaceId === null) return undefined + let surfaceId: string | undefined = String(rawSurfaceId).trim() + if (!surfaceId) return undefined
♻️ Duplicate comments (8)
webui/src/Controls/OptionsInputField.tsx (1)
212-220: Potential null deref onlocalVariablesStore(duplicate).
Line 217 still useslocalVariablesStore!even though the prop allowsnull.companion/lib/Internal/BuildingBlocks.ts (1)
287-299: Lint still warns aboutexecution_modestringification (Line 298).Even if the value should be a string, the lint failure will keep biting; a defensive
String()avoids it.💡 Suggested fix
- this.#logger.error(`Unknown execution mode: ${action.options.execution_mode}`) + this.#logger.error(`Unknown execution mode: ${String(action.options.execution_mode)}`)companion/lib/Internal/CustomVariables.ts (1)
272-293: Lint still warns aboutString(action.options.name)(Line 274+).This is the same
[object Object]concern flagged earlier; consider narrowing to a string once and reusing it.💡 Suggested fix
if (action.definitionId === 'custom_variable_set_value') { - if (this.#variableController.custom.hasCustomVariable(String(action.options.name))) { - this.#variableController.custom.setValue( - String(action.options.name), - action.options.value as CompanionVariableValue - ) - } else if (action.options.create) { - this.#variableController.custom.createVariable( - String(action.options.name), - action.options.value as CompanionVariableValue - ) - } else { - this.#logger.warn(`Custom variable "${action.options.name}" not found`) - } + const name = action.options.name as string + if (this.#variableController.custom.hasCustomVariable(name)) { + this.#variableController.custom.setValue(name, action.options.value as CompanionVariableValue) + } else if (action.options.create) { + this.#variableController.custom.createVariable(name, action.options.value as CompanionVariableValue) + } else { + this.#logger.warn(`Custom variable "${name}" not found`) + } return true } else if (action.definitionId === 'custom_variable_reset_to_default') { - this.#variableController.custom.resetValueToDefault(String(action.options.name)) + this.#variableController.custom.resetValueToDefault(action.options.name as string) return true } else if (action.definitionId === 'custom_variable_sync_to_default') { - this.#variableController.custom.syncValueToDefault(String(action.options.name)) + this.#variableController.custom.syncValueToDefault(action.options.name as string) return truecompanion/lib/Internal/System.ts (1)
329-341: Upgrade uses the wrong key forcustom_log(Line 333).The option id is
message, so the upgrade won’t convert the right field.💡 Suggested fix
- if (action.definitionId === 'custom_log') { - changed = convertSimplePropertyToExpressionValue(action.options, 'custom_log') || changed + if (action.definitionId === 'custom_log') { + changed = convertSimplePropertyToExpressionValue(action.options, 'message') || changedcompanion/lib/Internal/Controls.ts (2)
942-960: Same lint issue withtrigger_id.💡 Suggested fix
} else if (action.definitionId === 'panic_trigger') { - const rawControlId = String(action.options.trigger_id) + const triggerValue = action.options.trigger_id + const rawControlId = typeof triggerValue === 'string' ? triggerValue : String(triggerValue ?? '')
896-926: Fix lint failure:String()on potentially objectaction.options.location.The static analysis flags that
action.options.locationcould be anExpressionOrValueobject. However, sinceoptionsSupportExpressions: trueand the parser runs beforeexecuteAction, the value should already be parsed to a string. But the linter doesn't know this.💡 Suggested fix to satisfy the linter
} else if (action.definitionId === 'panic_bank') { // Special case handling for special modes - const rawControlId = String(action.options.location).trim().toLowerCase() + const locationValue = action.options.location + const rawControlId = (typeof locationValue === 'string' ? locationValue : String(locationValue ?? '')).trim().toLowerCase()companion/lib/Internal/ActionRecorder.ts (1)
250-251: Fix lint failure:String()on potentially object values.The static analysis correctly flags that
action.options.stepandaction.options.setcould beExpressionOrValueobjects, which would stringify to"[object Object]". Since theactionUpgrademethod converts these toExpressionOrValue, the parsed value should already be a string after auto-parsing, but TypeScript doesn't know that.💡 Suggested fix
- let stepId = String(action.options.step) - let setId = String(action.options.set) + let stepId = String(action.options.step ?? '') + let setId = String(action.options.set ?? '')Or if the values might still be objects in edge cases:
- let stepId = String(action.options.step) - let setId = String(action.options.set) + const stepRaw = action.options.step + const setRaw = action.options.set + let stepId = typeof stepRaw === 'object' ? String(stepRaw?.value ?? '') : String(stepRaw ?? '') + let setId = typeof setRaw === 'object' ? String(setRaw?.value ?? '') : String(setRaw ?? '')companion/lib/Internal/Variables.ts (1)
348-367: Removenocommitcomment and dead code to unblock CI.The ESLint
no-warning-commentsrule is catching thenocommitcomment, which will block the build. The commented-out code block also appears to be remnants from the old implementation that should be cleaned up.If this is intentional WIP, consider converting to a regular
TODOcomment (without "nocommit") so CI can pass.💡 Suggested cleanup
`#updateLocalVariableValue`( action: ActionForInternalExecution, extras: RunActionExtras, updateValue: ( entityPool: ControlEntityListPoolBase, listId: SomeSocketEntityLocation, variableEntity: ControlEntityInstance ) => void ) { if (!action.options.name) return - // nocommit - this needs to be adjusted to handle triggers/expression variables + // TODO - this needs to be adjusted to handle triggers/expression variables const location = ParseLocationString(String(action.options.location), extras.location) const theControlId = location ? this.#pageStore.getControlIdAt(location) : null - - // let theControlId: string | null = null - // if (action.rawOptions.location_target === 'this') { - // // This could be any type of control (button, trigger, etc) - // theControlId = extras.controlId - // } else { - // // Parse the location of a button - // const result = this.#internalUtils.parseInternalControlReferenceForActionOrFeedback( - // extras, - // action.rawOptions, - // true - // ) - // theControlId = result.location ? this.#pageStore.getControlIdAt(result.location) : null - // } if (!theControlId) return
🧹 Nitpick comments (2)
webui/src/Controls/OptionsInputField.tsx (1)
65-80: Make ExpressionOrValue handling resilient to legacy raw values.
If a stored option is still a primitive,(rawValue as ExpressionOrValue).valuebecomesundefinedandFieldOrExpressioncan receive a non-object. A small guard keeps things safe during migration.♻️ Suggested guard
-import type { ExpressionOrValue, SomeCompanionInputField } from '@companion-app/shared/Model/Options.js' +import { isExpressionOrValue, type ExpressionOrValue, type SomeCompanionInputField } from '@companion-app/shared/Model/Options.js' @@ - const value = fieldsSupportExpressions ? (rawValue as ExpressionOrValue<any>)?.value : rawValue + const rawExpressionValue = isExpressionOrValue(rawValue) + ? rawValue + : { isExpression: false, value: rawValue } + const value = fieldsSupportExpressions ? rawExpressionValue.value : rawValuecompanion/lib/Internal/Controller.ts (1)
295-359: Feedback value retrieval with proper error handling.The try/catch/finally structure is a nice improvement for robustness. One small suggestion: in the catch block, you might want to clear
feedback.referencedVariablesto avoid stale variable subscriptions if parsing fails.💡 Suggested improvement
} catch (e: any) { + feedback.referencedVariables = null this.#logger.warn( `Feedback get value failed: ${JSON.stringify(feedback.entityModel)} - ${e?.message ?? e} ${e?.stack}` ) return undefined
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 (1)
companion/lib/Internal/Surface.ts (1)
130-163: Guard against missingsurfaceIdto avoid"undefined"lookups.
String(undefined)becomes"undefined", which is truthy and can trigger invalid surface operations. Consider early-returning on null/empty values.🛠️ Suggested fix
- let surfaceId: string | undefined = String(options.surfaceId).trim() + if (options.surfaceId == null) return undefined + let surfaceId: string | undefined = String(options.surfaceId).trim() + if (!surfaceId) return undefined
♻️ Duplicate comments (3)
companion/lib/Preview/Graphics.ts (1)
170-185: Removenocommitcomments to unblock lint.
CI fails theno-warning-commentsrule at Line 170 and Line 256. Please strip those inline notes.🧹 Suggested cleanup
- const locationValue = parser.parseEntityOption(options.location, 'variables') // nocommit - this looks wrong + const locationValue = parser.parseEntityOption(options.location, 'variables') ... - const locationValue = parser.parseEntityOption(previewSession.options.location, 'variables') // nocommit - this looks wrong + const locationValue = parser.parseEntityOption(previewSession.options.location, 'variables')Also applies to: 255-282
companion/lib/Internal/Variables.ts (1)
347-374:nocommitcomment still breaks lint — needs removal/rename.CI will fail due to
no-warning-comments. Switching to a normal TODO is enough.🛠️ Suggested fix
- // nocommit - this needs to be adjusted to handle triggers/expression variables + // TODO: this needs to be adjusted to handle triggers/expression variablescompanion/lib/Internal/System.ts (1)
333-345: Upgrade should targetmessage, notcustom_log.The option id is
message, so the current conversion is a no-op for existing actions.🛠️ Suggested fix
- changed = convertSimplePropertyToExpressionValue(action.options, 'custom_log') || changed + changed = convertSimplePropertyToExpressionValue(action.options, 'message') || changed
🧹 Nitpick comments (3)
shared-lib/lib/Model/Options.ts (1)
249-257: Consider strengthening the type guard slightly.The
isExpressionOrValueguard checks forisExpressionbeing a boolean, but doesn't verify thevalueproperty exists. While this likely works fine in practice, a stricter check could prevent edge cases where an object happens to have justisExpression.💡 Optional enhancement
export function isExpressionOrValue(input: any): input is ExpressionOrValue<any> { - return !!input && typeof input === 'object' && 'isExpression' in input && typeof input.isExpression === 'boolean' + return !!input && typeof input === 'object' && 'isExpression' in input && typeof input.isExpression === 'boolean' && 'value' in input }companion/lib/Internal/Triggers.ts (1)
125-151: Friendly heads-up: silent success on missing trigger ID.When
stringifyVariableValue(action.options.trigger_id)returns falsy, you returntrue(success). This is probably intentional to avoid breaking flows, but it might make debugging tricky if someone misconfigures their trigger.Would it be helpful to log a warning here, similar to what's done in
CustomVariables.tswith the"Custom variable not found"warning? Totally optional, just a thought!companion/lib/Internal/Util.ts (1)
9-75: The TODOs acknowledge this needs love - happy to help when you're ready!The
ParseLocationStringfunction works, but I noticed the TODOs on lines 44 and 46 calling out that this parsing is "horrible" and needs reworking. 😄The nested conditionals and multiple format support (legacy bank IDs, row/column, page/row/column) make this quite complex. When you're ready to tackle simplification, feel free to reach out - happy to help brainstorm a cleaner approach!
One small observation: if
parts[0]is something like"abc"(non-numeric),Number(parts[0])will returnNaN, whichsanitisePageNumberpasses through. This might cause unexpected behavior downstream.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
companion/lib/Internal/Util.ts (1)
145-149: Escape backticks/${}when embedding variable strings.If
options[keys.variable]contains backticks or${, the generated template literal can break parsing. Consider escaping those characters.🔧 Suggested hardening
- const variableName = stringifyVariableValue(options[keys.variable]) + const variableName = stringifyVariableValue(options[keys.variable]) options[keys.result] = { isExpression: true, - value: !variableName ? '' : `parseVariables(\`${variableName}\`)`, + value: !variableName ? '' : `parseVariables(\`${String(variableName).replace(/[`\\$]/g, '\\$&')}\`)`, } satisfies ExpressionOrValue<string>companion/lib/Internal/Controls.ts (1)
146-154: Avoid stringifying ExpressionOrValue objects.Line 153 uses
String(options.location), which can yield"[object Object]"ifoptions.locationis still wrapped. Unwrapping viastringifyVariableValuekeeps this safe.✅ Suggested fix
- const location = ParseLocationString(String(options.location), extras.location) + const location = ParseLocationString(stringifyVariableValue(options.location) ?? '', extras.location)companion/lib/Variables/VariablesAndExpressionParser.ts (1)
189-189: Please remove thenocommitmarker (lint failure).Line 189 violates the
no-warning-commentsrule and currently fails CI.🧹 Suggested cleanup
- // nocommit - check value is valid according to the ruleswebui/src/Components/FieldOrExpression.tsx (1)
42-47: Coerce values when switching into expression mode.When toggling into expression mode,
value.valuecan be non‑string (number/boolean/object). That violatesExpressionOrValue’sisExpression: trueshape and can later break parsing if the user toggles without editing. Coerce to a string at the point of toggle.🔧 Proposed fix
const setIsExpression = useCallback( (isExpression: boolean) => { setValue({ isExpression, - value: value.value, + value: isExpression ? (stringifyVariableValue(value.value) ?? '') : value.value, }) }, [setValue, value] )
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
companion/lib/Controls/Controller.ts (1)
767-771: Nice type refinement fromanyto a specific union type!Replacing
anywithCompanionFeedbackButtonStyleResult | VariableValueis a welcome improvement for type safety. This aligns well with the expression-enabled option handling introduced in this PR.One small thing to consider for a future pass: Line 719 still uses
Record<string, Record<string, any>>for the intermediatevaluesobject. You might want to update that for consistency at some point, though it's not critical since the data flows into typed structures anyway.💡 Optional: Consider aligning the intermediate values type
- const values: Record<string, Record<string, any>> = {} + const values: Record<string, Record<string, CompanionFeedbackButtonStyleResult | VariableValue>> = {}This would make the typing consistent throughout the
updateFeedbackValuesmethod.
wip wip: controls wip: custom variables wip: action recorder wip: fix visitor wip: convert more actions wip: ux fix wip: types wip: finish internal surface actions/feedback wip: something starting to work wip: fb test fix: entitymanager parsing wip: refactor internal 'module' in preparation wip: refactor wip: refactor InstanceEntityManager
Julusian
force-pushed
the
feat/expressions-internal-actions
branch
from
f330e2a to
4319505
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
webui/src/Controls/OptionsInputField.tsx (1)
212-227: Potential null dereference onlocalVariablesStore.Hey there! 👋 At line 217,
localVariablesStoreis passed directly toFieldOrExpression, but the condition at line 212 only checksfieldsSupportExpressions, not whetherlocalVariablesStoreis non-null. If someone passesfieldSupportsExpression: truewith a nulllocalVariablesStore, this could cause issues at runtime.A small defensive check would make this more robust:
💡 Suggested safeguard
- } else if (fieldsSupportExpressions) { + } else if (fieldsSupportExpressions && localVariablesStore) { const rawExpressionValue = (rawValue as ExpressionOrValue<any>) || { isExpression: false, value: undefined } control = ( <FieldOrExpression - localVariablesStore={localVariablesStore} + localVariablesStore={localVariablesStore}
🧹 Nitpick comments (4)
companion/lib/Variables/CustomVariable.ts (1)
199-205: Align default-value typing and validation with the new signatureNice change! Since
createVariablenow acceptsVariableValue | undefined, the surrounding docstring and setter signatures still mentionstring, and the TRPCcreateinput still enforcesz.string(). If the intent is to allow non-string defaults, consider updating the doc comment andsetVariableDefaultValuesignature (and optionally relaxing/validating the TRPC input) so the public surface is consistent. If strings are still the only allowed defaults, it may be clearer to keep the signaturestringand coerce/validate at the boundary instead.companion/lib/Internal/CustomVariables.ts (1)
107-269: Comprehensive upgrade logic for legacy actions.This is a thorough migration path that consolidates many legacy action types (math operations, string operations, JSON path, etc.) into the unified
custom_variable_set_valueaction. The expression generation for each legacy type looks correct.One thing to consider: the
wrapValuehelper (lines 109-117) uses double quotes for non-variable string wrapping inparseVariables("${val}"). Ifvalcontains double quotes, this could break the generated expression. You might want to escape them:💡 Suggested improvement
const wrapValue = (val: string | number) => { if (!isNaN(Number(val))) { return Number(val) } else if (typeof val === 'string' && val.trim().match(variableRegex)) { return val.trim() } else { - return `parseVariables("${val}")` + return `parseVariables("${String(val).replace(/"/g, '\\"')}")` } }companion/lib/Internal/Instance.ts (1)
352-420: Feedback execution with type casts.The
as anycasts for color properties (e.g.,feedback.options.error_fg as any) are necessary becauseCompanionOptionValuestypes options asunknown. This is a pragmatic approach.Consider adding a brief comment explaining why the casts are needed, to help future maintainers:
// Color options are typed as unknown in CompanionOptionValues, cast needed color: feedback.options.error_fg as any,companion/lib/Internal/Controls.ts (1)
821-978: Action execution handles special modes and color parsing.The
panic_bankhandling (lines 885-915) properly checks for special modes (this-run,this-all-runs) before falling back to location parsing.For color actions, using
parseColorToNumber(action.options.color as any) || 0provides a safe default of 0 (black) if parsing fails.One small suggestion for
button_text(line 881): you're passingaction.options.labeldirectly tostyleSetFields. If this could be a non-string value, you might want to stringify it:💡 Suggested safeguard
if (control && control.supportsStyle) { - control.styleSetFields({ text: action.options.label }) + control.styleSetFields({ text: stringifyVariableValue(action.options.label) ?? '' }) }
It was half replaced during #3617, but got restored due to some merge conflicts
This is a WIP, and is in the process of being applied to all the internal code, please raise any objections/concerns around the changes asap, it would be good to know if anything needs reworking before every internal action is updated and tested for the changes.This starts on implementing #2345, starting with considering just the internal actions and feedbacks.
The aim is to figure out how this will work technically, while it is easier to do on just the code in this repo before attempting to roll it out to modules.
The ui looks pretty similar to what was done for the graphics overhaul. Fields which support this gain a new button to the right of the field, which toggles it between the usual mode and expressions:
Internally, the options object now contains something of the form:
{ isExpression: false, value: 123 }, with the execution auto-parsing, and the ui updating this. It is opt-in per entity for now, to avoid breaking everything while this is WIP, that may or may not remain once complete.This data change requires upgrade scripts per-action. While it probably could be mostly automated, many actions need input fields to also be combined (they already have a checkbox to toggle between value and expression mode, and separate fields for each mode), so doing it all manually makes handling this saner/safer
Summary by CodeRabbit
New Features
Refactor
Tests / Docs
✏️ Tip: You can customize this high-level summary in your review settings.