feat: convert value when toggling between expression and raw mode #4087

[Julusian]

closes #4087

When toggling a field between expression and value mode, this will now explicitly convert it between the expression string and the value.

If it encounters a plain value, the field will be switched quietly, but if it contains an expression doing anything more than just a literal value/object, then it will show a modal to require confirmation before replacing the expression the user has written

Not 100% happy with the styling of the modal, but its better than nothing:

Summary by CodeRabbit

• New Features

  • Added an expression-to-value conversion modal that evaluates expressions and lets users apply the computed value when switching from expression to value mode.
  • Field editing now attempts to auto-convert plain expression literals to values and prompts when needed.

• Tests

  • Added tests covering extraction of plain expression values and cases where conversion is not possible.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3210dc56-6f27-4348-b96d-b171ff287c07

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds two expression helpers for literal conversion and static extraction, a modal to preview/confirm converting expressions to values using a live subscription, and integrates extraction + modal into field components so toggling from expression to value preserves plain literals when possible.

Changes

Cohort / File(s)	Summary
Expression Parsing Utilities shared-lib/lib/Expression/ExpressionParse.ts, shared-lib/lib/__tests__/expressions-parse.test.ts	Exported `valueToExpressionLiteral(value: JsonValue
Expression Conversion UI webui/src/Components/ExpressionConversionModal.tsx	New ExpressionConversionModal React component that subscribes to a preview stream, shows evaluating/loading/error states, displays computed value, and calls onConfirm/onCancel with the latest subscription result.
Field Expression Integration webui/src/Components/FieldOrExpression.tsx, webui/src/Controls/OptionsInputField.tsx	Plumbed `controlId: string

Poem

✨ Expressions whisper secrets, plain and bright,
🔍 We parse the literals, bring them to light.
🪟 A little modal asks, "Shall I convert?"
✅ Confirm the value — no surprises or hurt.
🎉 Small comforts for toggles, tidy and right.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main feature: converting values when toggling between expression and raw modes, directly addressing issue #4087.
Linked Issues check	✅ Passed	The PR successfully implements the core requirement from #4087: when toggling between expression and value modes, it now converts values appropriately—extracting plain values silently or showing a confirmation modal for complex expressions.
Out of Scope Changes check	✅ Passed	All changes are directly related to the stated objective of handling value/expression mode conversions. New helper functions, tests, modal component, and field integration all support this goal without introducing unrelated functionality.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (4)

shared-lib/lib/Expression/ExpressionParse.ts (1)

50-54: Consider handling undefined explicitly since it's not a valid JsonValue.

The comment on line 52 mentions that undefined is patched in by fixupExpression. Since undefined is technically not part of the JsonValue type from type-fest, you might want to handle it explicitly to make the type assertion cleaner. That said, the current approach will work in practice since the consuming code in FieldOrExpression handles JsonValue | undefined.

Just a thought — if you'd prefer to keep it as-is for simplicity, that's totally fine too! 🙂

💡 Optional: Explicit undefined handling

 export function tryExtractExpressionPlainValue(node: SomeExpressionNode): { value: JsonValue } | null {
 	if (node.type === 'Literal') {
-		// jsep Literal values are string | number | boolean | null (undefined is patched in by fixupExpression)
-		return { value: node.value as JsonValue }
+		// jsep Literal values are string | number | boolean | null | undefined
+		// undefined is patched in by fixupExpression for the `undefined` identifier
+		const literalValue = node.value
+		if (literalValue === undefined) return { value: null }
+		return { value: literalValue as JsonValue }
 	}

shared-lib/lib/__tests__/expressions-parse.test.ts (1)

1137-1202: Excellent test coverage for the plain value extraction! 🎉

The tests thoroughly cover the happy path scenarios including edge cases like zero, empty arrays/objects, and nested structures. Really nice work here!

One small suggestion — you might consider adding a test case for the undefined literal since fixupExpression patches that in as a special case:

it('undefined', () => {
	expect(tryExtractExpressionPlainValue(ParseExpression('undefined'))).toEqual({ value: undefined })
})

This would help document the expected behavior and catch any regressions if the undefined handling changes.

webui/src/Components/ExpressionConversionModal.tsx (2)

52-56: Consider disabling the confirm button while evaluation is in progress.

If the user clicks "Use computed value" before the subscription has emitted any data, latestDataRef.current will be undefined, and the confirm will set the value to undefined. This might be unexpected behavior — the user might think they're getting the computed result.

You could either disable the button until data arrives, or show a more explicit warning. Just a thought to improve the UX! 🙂

💡 Suggested improvement

 			<CButton color="secondary" onClick={onCancel}>
 				Cancel
 			</CButton>
-			<CButton color="primary" onClick={doConfirm}>
+			<CButton color="primary" onClick={doConfirm} disabled={!displayData}>
 				Use computed value
 			</CButton>

---

96-103: Small observation about button styling.

The PR description image shows a red "Use computed value" button, but the code uses color="primary" (typically blue in CoreUI). If the red styling was intentional (perhaps to emphasize that this is a destructive action), you might want to use color="danger" instead. But if the screenshot is just outdated mockup, no worries!

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1b9f5244-a4e0-468d-ad56-7030d4e95665

📥 Commits

Reviewing files that changed from the base of the PR and between a5e52a5 and 25abd67.

📒 Files selected for processing (5)

• shared-lib/lib/Expression/ExpressionParse.ts
• shared-lib/lib/__tests__/expressions-parse.test.ts
• webui/src/Components/ExpressionConversionModal.tsx
• webui/src/Components/FieldOrExpression.tsx
• webui/src/Controls/OptionsInputField.tsx

[phillipivan]

Should the modal have 3rd option beyond cancel and use computed value? I feel like there should be; like a select / enter alternate value. But I don't know of the top of my head when one might want to use this.

[thedist]

I feel like there should be; like a select / enter alternate value

Wouldn't that just be cancelling and the user entering a different value?

[Julusian]

I think this needs some more thought, and trial and error and I dont want to delay 4.3.0 for it. hopefully it can be done for 4.3.1

After looking into this more, there is some ambiguity on how it should behave:

• for a textinput of abc $(abc:test) going to an expression it is reasonable to expect "abc $(abc:test)", but perhaps it would be expected to remain abc $(abc:test) (which is how the button text behaves today).
• doing the reverse for a text field, maybe the user started typing before realising they were in expression mode
• For a number/checkbox, this very much does make sense
• For a dropdown without allowCustom: true, this would also make sense to do; but perhaps it should be a bit looser so that either abc or "abc" in the expression field will become abc
• For a dropdown with allowCustom: true, maybe different in some way?
• maybe I overlooked some types, perhaps for internal field types
• have I handled the case where the expression itself is invalid?

And I am not entirely happy with how this modal is looking, I am not sure how to present the bits of data in a friendly way. it should contain some amount of:

• the expression
• if the computed value is 'invalid'
  • the reason it is invalid
  • the invalid value?
  • the value that will be used when clicking 'use default value'
• if the computed value is 'valid'
  • any warnings returned by the validator
  • the sanitised value that will be used

So I am thinking that a first iteration of this could skip touching textinput fields, but there is still some unknowns and testing that needs to be tackled for the rest and I don't want to delay 4.3.0 any longer than it already is

If anyone feels like doing some sketches/patches for the layout of this modal, I welcome the input; that is the part that I will feel blocked on shortly.

[Julusian]

A note for me to return to; this was on develop for some graphics field and should be used for inspiration on transforms here

// Fetch the property wrapper
		const elementEntry = (element as any)[key] as ExpressionOrValue<JsonValue | undefined>
		if (!elementEntry) return false

		if (!elementEntry.isExpression && value) {
			// Make sure the value is expression safe
			if (element.type === 'text' && key === 'text') {
				// Skip, this is very hard to fixup perfectly
			} else if (typeof elementEntry.value === 'string') {
				// If its a string, it will need to be wrapped in quotes
				// This is not always good enough, but is better than nothing
				elementEntry.value = `'${elementEntry.value}'`
			} else if (typeof elementEntry.value === 'number' && key === 'color') {
				// If its a color number, it is nicer to have it as a hex string
				elementEntry.value = '0x' + elementEntry.value.toString(16)
			} else if (typeof elementEntry.value === 'boolean') {
				elementEntry.value = elementEntry.value ? 'true' : 'false'
			}
			// Future: this may want more cases
		} else if (elementEntry.isExpression && !value) {
			// Preserve current resolved value
			const lastDrawStyle = this.getLastDrawStyle()
			const lastDrawElement = lastDrawStyle?.elements.find((el) => el.id === id)

			if (key === 'enabled' && !lastDrawElement) {
				// Special case, element was presumably disabled
				elementEntry.value = false as any
			} else if (lastDrawElement) {
				// The element was found, copy the value
				elementEntry.value = lastDrawElement[key as keyof typeof lastDrawElement]
			}
		}