Re: [PATCH proxmox-widget-toolkit v7 21/41] notifications: matcher: add support for match expressions

[Dominik Csapak <d.csapak@proxmox.com>]

hi, looks mostly good to me, a few nits and comments inline

no blockers for me though
(maybe only consider them as follow up improvements or if
you need to send another version)

In general i'm not a fan of the viewmodel to be declared and modified
in one component and the effects only applied in another
(e.g. which match field is shown)

Nothing tragic (and i think it's pre-existing),
but the connection between components is often lost when one
does not know what to look for....

On 11/3/26 11:57 AM, Lukas Wagner wrote:
> This patches changes to 'matcher edit window' to support the new
> 'expression' paramter in the matcher config. Instead of of flat
> 'match-*' configuration keys, 'expression' contains the match rules as a
> serialized JSON blob. This allows us to create arbitrarily nested match
> formulas.
> 
> Support for match expressions has to be enabled via a feature flag. This
> avoids awkward versioned breaks between the widget toolkit package and
> the main product.
> 
> The entire match rule panel has changed so much that copying the old
> component and using that as a base was deemed more useful than modifying
> the existing panel in-place. The old component can be removed once we
> have migrated all products to this new system.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> Reviewed-by: Michael Köppl <m.koeppl@proxmox.com>
> Tested-by: Michael Köppl <m.koeppl@proxmox.com>
> ---
> 
> Notes:
>      Changes since v1:
>        - Don't allow empty all-of/any-of/one-of rules, show an
>          error below the tree then
>      
>      Changes since v2:
>        - properly handle nested 'not' expressions.
>          While the UI never produces them, they could be added
>          via the API or CLI, so the UI must understand them correctly.
>          The UI will automatically flatten/simply these on update.
>      
>      Changes since v3:
>        - remove left-over console.log statement
> 
>   src/Makefile                                  |   1 +
>   src/Schema.js                                 |  10 +
>   src/css/ext6-pmx.css                          |  51 +
>   .../NotificationMatchExpressionEditPanel.js   | 872 ++++++++++++++++++
>   src/proxmox-dark/scss/extjs/_treepanel.scss   |   4 +
>   src/proxmox-dark/scss/proxmox/_general.scss   |   4 +
>   src/window/NotificationMatcherEdit.js         |  13 +-
>   7 files changed, 950 insertions(+), 5 deletions(-)
>   create mode 100644 src/panel/NotificationMatchExpressionEditPanel.js
> 
> diff --git a/src/Makefile b/src/Makefile
> index 9c52339..f11716a 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -64,6 +64,7 @@ JSSRC=					\
>   	panel/LogView.js		\
>   	panel/NodeInfoRepoStatus.js	\
>   	panel/NotificationConfigView.js	\
> +	panel/NotificationMatchExpressionEditPanel.js	\
>   	panel/JournalView.js		\
>   	panel/PermissionView.js		\
>   	panel/PruneKeepPanel.js		\
> diff --git a/src/Schema.js b/src/Schema.js
> index 5de3f53..e72d301 100644
> --- a/src/Schema.js
> +++ b/src/Schema.js
> @@ -86,6 +86,16 @@ Ext.define('Proxmox.Schema', {
>           }
>       },
>   
> +    notificationMatcherExpressions: false,
> +
> +    notificationMatcherPanelType: function () {
> +        if (Proxmox.Schema.notificationMatcherExpressions) {
> +            return 'pmxNotificationMatchExpressionEditPanel';
> +        } else {
> +            return 'pmxNotificationMatchRulesEditPanel';
> +        }
> +    },
> +
>       pxarFileTypes: {
>           b: { icon: 'cube', label: gettext('Block Device') },
>           c: { icon: 'tty', label: gettext('Character Device') },
> diff --git a/src/css/ext6-pmx.css b/src/css/ext6-pmx.css
> index 878dfa0..93010e6 100644
> --- a/src/css/ext6-pmx.css
> +++ b/src/css/ext6-pmx.css
> @@ -248,6 +248,53 @@ div.right-aligned {
>       background-color: #f5f5f5;
>   }
>   
> +/* the small icons */
> +.x-tree-icon-custom:after {
> +    position: relative;
> +    left: -5px;
> +    top: 1px;
> +    font-size: 0.75em;
> +    text-shadow: -1px 0px 2px #fff;
> +    content: "\ ";
> +}
> +
> +/* yellow ! triangle */
> +.x-tree-icon-custom.internal-error:after {
> +    content: "\f071";
> +    color: #ffcc00;
> +}
> +
> +.x-tree-node-text > code {
> +    display: inline-block;
> +    background-color: #f5f5f5;
> +    border-radius: 4px;
> +    padding: 1px 4px;
> +    margin: 1px 0;
> +    font-size: 11px;
> +    /* a single long value, e.g. a regex, has to break instead of overflowing */
> +    overflow-wrap: anywhere;
> +}

we could probably have a follow up removing them in pve-manager and 
proxmox-backup ?

> +
> +/* The notification matcher rule tree wraps its labels, see 'cellWrap'. Laying
> + * the cell out as a flex row keeps the wrapped lines aligned with the first
> + * one, instead of restarting at the left edge under the indentation. */
> +.pmx-match-rule-tree .x-grid-cell-inner-treecolumn {
> +    display: flex;
> +    align-items: flex-start;
> +}
> +
> +/* indentation, elbows and the node icon have a fixed size and must not shrink */
> +.pmx-match-rule-tree .x-tree-elbow-img,
> +.pmx-match-rule-tree .x-tree-icon {
> +    flex: none;
> +}
> +
> +/* 'min-width' lets the label get narrower than its longest value */
> +.pmx-match-rule-tree .x-tree-node-text {
> +    flex: 1;
> +    min-width: 0;
> +}
> +
>   /* fix padding for legend in header */
>   .x-legend-inner {
>       padding: 0;
> @@ -431,3 +478,7 @@ div.right-aligned {
>       font-style: italic;
>   }
>   /* journal log level coloring end */
> +
> +.pmx-horizontal-separator {
> +    border-top: 1px solid #cfcfcf;
> +}
> diff --git a/src/panel/NotificationMatchExpressionEditPanel.js b/src/panel/NotificationMatchExpressionEditPanel.js
> new file mode 100644
> index 0000000..ec9fd3c
> --- /dev/null
> +++ b/src/panel/NotificationMatchExpressionEditPanel.js
> @@ -0,0 +1,872 @@
> +Ext.define('Proxmox.panel.NotificationMatchExpressionEditPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    xtype: 'pmxNotificationMatchExpressionEditPanel',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    controller: {
> +        xclass: 'Ext.app.ViewController',
> +
> +        // we want to also set the empty value, but 'bind' does not do that so
> +        // we have to set it then (and only then) to get the correct value in
> +        // the tree
> +        control: {
> +            field: {
> +                change: function (cmp) {
> +                    let me = this;
> +                    let vm = me.getViewModel();
> +                    if (cmp.field) {
> +                        let record = vm.get('selectedRecord');
> +                        if (!record) {
> +                            return;
> +                        }
> +                        let data = Ext.apply({}, record.get('data'));
> +                        let value = cmp.getValue();
> +                        // only update if the value is empty (or empty array)
> +                        if (!value || !value.length) {
> +                            data[cmp.field] = value;
> +                            record.set({ data });
> +                        }
> +                    }
> +                },
> +            },
> +        },
> +    },
> +
> +    viewModel: {
> +        data: {
> +            invertMatch: false,
> +            selectedRecord: null,
> +            matchFieldType: 'exact',
> +            matchFieldField: '',
> +            matchFieldValue: '',
> +        },
> +
> +        formulas: {
> +            showMatcherType: (get) => get('selectedRecord') !== null,
> +
> +            invertMatch: {
> +                bind: {
> +                    bindTo: '{selectedRecord}',
> +                    deep: true,
> +                },
> +                get: function (record) {
> +                    return record?.get('invert');
> +                },
> +                set: function (value) {
> +                    let me = this;
> +                    let record = me.get('selectedRecord');
> +                    record.set('invert', value);
> +                },
> +            },
> +            nodeType: {
> +                get: function (get) {
> +                    let record = get('selectedRecord');
> +                    return record?.get('type');
> +                },
> +                set: function (value) {
> +                    let me = this;
> +                    let record = me.get('selectedRecord');
> +
> +                    let data;
> +                    let leaf;
> +
> +                    switch (value) {
> +                        case 'match-severity':
> +                            data = {
> +                                value: ['info', 'notice', 'warning', 'error', 'unknown'],
> +                            };
> +                            leaf = true;
> +                            break;
> +                        case 'match-field':
> +                            data = {
> +                                type: 'exact',
> +                                field: '',
> +                                value: '',
> +                            };
> +                            leaf = true;
> +                            break;
> +                        case 'match-calendar':
> +                            data = {
> +                                value: '',
> +                            };
> +                            leaf = true;
> +                            break;
> +                        case 'any-of':
> +                            data = {};
> +                            leaf = false;
> +                            break;
> +                        case 'all-of':
> +                            data = {};
> +                            leaf = false;
> +                            break;
> +                        case 'one-of':
> +                            data = {};
> +                            leaf = false;
> +                            break;

nit: these could be combined:

case 'any-of':
case 'all-of':
case 'one-of':
  ...
  break;
> +                        case 'match-always':
> +                            value = 'match-always';

nit: we match on value for 'match-always' and set it to 'match-always' ?
some leftover probably

> +                            data = {};
> +                            leaf = true;
> +                            break;
> +                    }
> +
> +                    let node = {
> +                        type: value,
> +                        invert: false,
> +                        data,
> +                        leaf,
> +                    };
> +
> +                    if (!leaf) {
> +                        node.expanded = true;
> +                        node.expandable = false;
> +                    } else {
> +                        record.removeAll();
> +                    }
> +
> +                    record.set(node);
> +                },
> +            },
> +        },
> +    },
> +
> +    column1: [
> +        {
> +            xtype: 'pmxNotificationMatchRuleTreeExpression',
> +            cbind: {
> +                baseUrl: '{baseUrl}',
> +            },
> +        },
> +    ],
> +
> +    column2: [
> +        {
> +            xtype: 'pmxNotificationMatchExpressionNodePanel',
> +            cbind: {
> +                baseUrl: '{baseUrl}',
> +            },
> +        },
> +    ],
> +
> +    onGetValues: function (values) {
> +        let me = this;
> +
> +        if (!me.isCreate) {
> +            Proxmox.Utils.assemble_field_data(values, { delete: 'match-field' });
> +            Proxmox.Utils.assemble_field_data(values, { delete: 'match-severity' });
> +            Proxmox.Utils.assemble_field_data(values, { delete: 'match-calendar' });
> +            Proxmox.Utils.assemble_field_data(values, { delete: 'mode' });
> +            Proxmox.Utils.assemble_field_data(values, { delete: 'invert-match' });
> +        }
> +
> +        return values;
> +    },
> +});
> +
> +Ext.define('Proxmox.panel.NotificationMatchRuleTreeExpression', {
> +    extend: 'Ext.panel.Panel',
> +    xtype: 'pmxNotificationMatchRuleTreeExpression',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +    border: false,
> +
> +    // Returns a message describing why a node is incomplete, or undefined if
> +    // the node is fully configured. Incomplete nodes are marked in the tree and
> +    // block submitting the form.
> +    getNodeError: function (type, data, hasChildren) {
> +        switch (type) {
> +            case 'match-severity': {
> +                let v = Ext.isArray(data.value) ? data.value.join(', ') : data.value;
> +                if (!v) {
> +                    return gettext('Severity rules must match at least one severity.');
> +                }
> +                break;

nit: we don't need the result of 'v' anywhere just if it exists, so we
don't need to assemble with `.join(', ')` ?

just:

let has_value = Ext.isArray(data.value) ? data.value.length > 0 : 
!!data.value;

if (!has_value) {
    ...
}

(or have a similar check as below)

> +            }
> +            case 'match-field': {
> +                let field = data.field;
> +                let value = data.value;
> +                if (!field || !value || (Ext.isArray(value) && !value.length)) {

basically what i suggested above (sans boolean reversal)

> +                    return gettext('Field rules must have a field and a value.');
> +                }
> +                break;
> +            }
> +            case 'match-calendar': {
> +                let v = data.value;
> +                if (!v || !v.length) {

here is the third variant of that check,
these should probably be unified/refactored


let hasValue = (data) => Ext.isArray(data?.value) ? data.value.length > 
0: !!data?.value;


for instance (untested)

> +                    return gettext('Calendar rules must have a schedule.');
> +                }
> +                break;
> +            }
> +            case 'any-of':
> +            case 'all-of':
> +            case 'one-of':
> +                if (!hasChildren) {
> +                    return gettext('Rule groups must contain at least one sub-rule.');
> +                }
> +                break;
> +        }
> +
> +        return undefined;
> +    },
> +
> +    getNodeIcon: function (type, data, invert, hasChildren) {
> +        let me = this;
> +        let iconCls = 'fa';
> +
> +        switch (type) {
> +            case 'match-severity':
> +                iconCls += ' fa-exclamation';
> +                break;
> +            case 'match-field':
> +                iconCls += ' fa-square-o';
> +                break;
> +            case 'match-calendar':
> +                iconCls += ' fa-calendar-o';
> +                break;
> +            case 'any-of':
> +            case 'all-of':
> +            case 'one-of':
> +                iconCls += ' fa-filter';
> +                break;
> +            case 'match-always':
> +                if (invert) {
> +                    iconCls += ' fa-ban';
> +                } else {
> +                    iconCls += ' fa-check-circle';
> +                }
> +                break;
> +        }
> +
> +        if (me.getNodeError(type, data, hasChildren)) {
> +            iconCls += ' internal-error';
> +        }
> +
> +        return iconCls;
> +    },
> +
> +    renderTreeLabel: function (_value, meta, record) {
> +        let data = record.get('data');
> +        let invert = record.get('invert');
> +        let type = record.get('type');
> +
> +        switch (type) {
> +            case 'match-severity': {
> +                let v;
> +
> +                if (data.value) {
> +                    v = data.value.map((x) => `<code>${Ext.String.htmlEncode(x)}</code>`).join(' ');
> +                } else {
> +                    v = '<code>?</code>';
> +                }
> +
> +                if (invert) {
> +                    return Ext.String.format(gettext('Severity: all except {0}'), v);
> +                } else {
> +                    return Ext.String.format(gettext('Severity: {0}'), v);
> +                }
> +            }
> +            case 'match-field': {
> +                let valueIsArray = Ext.isArray(data.value);
> +
> +                let field = data.field ? data.field : '?';
> +                let value =
> +                    (!valueIsArray && data.value) || (valueIsArray && data.value.length)

here is even a fourth variant of that check from above ;)

maybe it's not a good idea to mix nonarray and array values on the same 
field?

we could have a 'value' field and 'arrayValue' field and use them 
separately?

also in error case this is always the '?' string, which reads odd..


> +                        ? data.value
> +                        : '?';
> +                let matchType = data.type;
> +
> +                let fieldHtml = `<code>${Ext.String.htmlEncode(field)}</code>`;
> +                let valueHtml = `<code>${Ext.String.htmlEncode(value)}</code>`;

here 'value' can be an array, this will be joined with ','. is that 
intended? wouldn't it be better to move the check length > 1 here and 
produce a different valueHtml upfront? then the branches below disappear?

> +
> +                if (matchType === 'exact') {
> +                    if (Ext.isArray(value) && value.length > 1) {

the array check already exists in 'valueIsArray', it was a different
'value' technically but couldn't we reuse that result here?

(the length == 0 case behaves differently then, but we could  move that 
length check into valueIsArray)

> +                        valueHtml = value
> +                            .map((x) => `<code>${Ext.String.htmlEncode(x)}</code>`)
> +                            .join(' ');
> +
> +                        if (invert) {
> +                            // inverted field matcher, matching multiple values
> +                            return Ext.String.format(
> +                                gettext('Field: {0} none of {1}'),
> +                                fieldHtml,
> +                                valueHtml,
> +                            );
> +                        } else {
> +                            // non-inverted field matcher, matching multiple values
> +                            return Ext.String.format(
> +                                gettext('Field: {0} any of {1}'),
> +                                fieldHtml,
> +                                valueHtml,
> +                            );
> +                        }
> +                    } else if (invert) {
> +                        // inverted field matcher, matching a single value
> +                        return Ext.String.format(
> +                            gettext('Field: {0} is not {1}'),
> +                            fieldHtml,
> +                            valueHtml,
> +                        );
> +                    } else {
> +                        // non-inverted field matcher, matching a single value
> +                        return Ext.String.format(
> +                            gettext('Field: {0} is {1}'),
> +                            fieldHtml,
> +                            valueHtml,
> +                        );
> +                    }
> +                } else if (invert) {
> +                    // inverted regex-based field matcher
> +                    return Ext.String.format(
> +                        gettext('Field: {0} does not match {1}'),
> +                        fieldHtml,
> +                        valueHtml,
> +                    );
> +                } else {
> +                    // non-inverted regex-based field matcher
> +                    return Ext.String.format(
> +                        gettext('Field: {0} matches {1}'),
> +                        fieldHtml,
> +                        valueHtml,
> +                    );
> +                }
> +            }
> +            case 'match-calendar': {
> +                let value = data.value ? data.value : '?';

this could just be

let value = data.value || '?';

or ? (reads a bit cleaner)

> +                value = `<code>${Ext.String.htmlEncode(value)}</code>`;

we do have basically the same snippet in 3 branches here,

unifying them would be nice

> +
> +                if (invert) {
> +                    return Ext.String.format(gettext('Calendar: outside {0}'), value);
> +                } else {
> +                    return Ext.String.format(gettext('Calendar: within {0}'), value);
> +                }
> +            }
> +            case 'any-of':
> +                if (invert) {
> +                    return gettext('No sub-rule matches');
> +                } else {
> +                    return gettext('At least one sub-rule matches');
> +                }
> +
> +            case 'all-of':
> +                if (invert) {
> +                    return gettext('At least one sub-rule does not match');
> +                } else {
> +                    return gettext('All sub-rules match');
> +                }
> +
> +            case 'one-of':
> +                if (invert) {
> +                    return gettext('Not: Exactly one sub-rule matches');
> +                } else {
> +                    return gettext('Exactly one sub-rule matches');
> +                }
> +
> +            case 'match-always':
> +                if (invert) {
> +                    return gettext('Never match');
> +                } else {
> +                    return gettext('Always match');
> +                }
> +        }
> +    },

imho the big switch with nested ifs is really hard to follow.

i would find it better to have the gettext declared
semantically e.g. in a map or as a return value of a function
(together with the number of parameters), like:

labelGetText: function(type, value, invert) {
     switch (type) {
         case 'match-severity':
             return [0, invert ? gettext('A') : gettext('B')]
	case '....':
	    ...
     }
}

then the whole method here would collapse to :

extract type, value;
generate valueHtml, fieldHtml
let [params, label] = labelGetText(...);

if (params === 0) {
     return label;
} elseif (params === 1) {
     return Ext.String.format(label, valueHtml);
} ...

> +
> +    initComponent: function () {
> +        let me = this;
> +
> +        let treeStore = Ext.create('Ext.data.TreeStore', {
> +            root: {
> +                expanded: true,
> +                expandable: false,
> +                text: '',
> +                type: 'match-always',
> +                data: {},
> +                invert: false,
> +                leaf: true,
> +                children: [],
> +                iconCls: 'fa fa-filter',
> +            },
> +        });
> +
> +        // The field holding the expression is hidden, so render its validation
> +        // errors below the tree instead.
> +        let errorLabel = Ext.create({
> +            xtype: 'component',
> +            hidden: true,
> +            cls: 'x-form-invalid-under-default',
> +        });
> +
> +        let getExpressionErrors = function () {
> +            let errors = [];
> +
> +            treeStore.getRoot().cascade(function (node) {
> +                let error = me.getNodeError(
> +                    node.get('type'),
> +                    node.get('data'),
> +                    node.hasChildNodes(),
> +                );
> +
> +                if (error && !errors.includes(error)) {
> +                    errors.push(error);
> +                }
> +            });
> +
> +            return errors;
> +        };
> +
> +        let realExpression = Ext.create({
> +            xtype: 'hiddenfield',
> +            name: 'expression',
> +            getErrors: function () {
> +                let errors = getExpressionErrors();
> +
> +                errorLabel.setHtml(
> +                    errors.map((error) => Ext.String.htmlEncode(error)).join('<br>'),
> +                );
> +                errorLabel.setHidden(!errors.length);
> +
> +                return errors;
> +            },
> +            setValue: function (value) {
> +                this.value = value;
> +                this.checkChange();
> +            },
> +            getValue: function () {
> +                return this.value;
> +            },
> +            getSubmitValue: function () {
> +                let value = this.value;
> +                return value;

nit: this could also just be `return this.value;` or not?

> +            },
> +        });
> +
> +        let storeChanged = function (store) {
> +            store.suspendEvent('datachanged');
> +
> +            store.each(function (model) {
> +                let type = model.get('type');
> +                let data = model.get('data');
> +                let invert = model.get('invert');
> +
> +                let iconCls = me.getNodeIcon(type, data, invert, model.hasChildNodes());
> +
> +                model.set({
> +                    iconCls,
> +                });
> +            });
> +
> +            let root = store.getRoot();
> +            let stack = [[]];
> +
> +            root.cascade({
> +                before: function (node) {
> +                    let type = node.get('type');
> +                    let data = node.get('data');
> +                    let invert = node.get('invert');
> +
> +                    switch (type) {
> +                        case 'all-of':
> +                        case 'any-of':
> +                        case 'one-of':
> +                            stack.push([]);
> +                            break;
> +                        case 'match-calendar':
> +                            {
> +                                let obj = {
> +                                    match: {
> +                                        type: 'calendar',
> +                                        schedule: data.value,
> +                                    },
> +                                };
> +
> +                                if (invert) {
> +                                    obj = {
> +                                        not: obj,
> +                                    };
> +                                }
> +
> +                                stack.at(-1).push(obj);
> +                            }
> +
> +                            break;
> +                        case 'match-field':
> +                            {
> +                                let obj = {
> +                                    match: {
> +                                        type: 'field',
> +                                        field: data.field,
> +                                    },
> +                                };
> +
> +                                if (data.type === 'exact') {
> +                                    obj.match.values = data.value;
> +                                } else {
> +                                    obj.match.regex = data.value;
> +                                }
> +
> +                                if (invert) {
> +                                    obj = {
> +                                        not: obj,
> +                                    };
> +                                }
> +
> +                                stack.at(-1).push(obj);
> +                            }
> +                            break;
> +                        case 'match-severity':
> +                            {
> +                                let obj = {
> +                                    match: {
> +                                        type: 'severity',
> +                                        severities: data.value,
> +                                    },
> +                                };
> +
> +                                if (invert) {
> +                                    obj = {
> +                                        not: obj,
> +                                    };
> +                                }
> +
> +                                stack.at(-1).push(obj);
> +                            }
> +                            break;
> +                        case 'match-always':
> +                            stack.at(-1).push({
> +                                constant: !invert,
> +                            });
> +                            break;
> +                    }
> +
> +                    // Visit this node
> +                    return true;
> +                },
> +                after: function (node) {
> +                    let type = node.get('type');
> +                    let invert = node.get('invert');
> +
> +                    switch (type) {
> +                        case 'all-of':
> +                        case 'any-of':
> +                        case 'one-of':
> +                            {
> +                                let children = stack.pop();
> +
> +                                let a = {};
> +                                a[type] = children;
> +
> +                                if (invert) {
> +                                    a = {
> +                                        not: a,
> +                                    };
> +                                }
> +
> +                                stack.at(-1).push(a);
> +                            }
> +                            break;
> +                    }
> +                },
> +            });
> +
> +            let topLevelNodes = stack.pop();
> +            let obj = topLevelNodes[0];
> +
> +            realExpression.suspendEvent('change');
> +            realExpression.setValue(JSON.stringify(obj));
> +            realExpression.resumeEvent('change');
> +
> +            // the expression can stay the same while the tree became (in)valid,
> +            // e.g. when the last child of a group is removed
> +            realExpression.validate();
> +
> +            store.resumeEvent('datachanged');
> +        };
> +
> +        realExpression.addListener('change', function (field, value) {
> +            let obj;
> +
> +            try {
> +                obj = JSON.parse(value);
> +            } catch (e) {
> +                Ext.Msg.alert(gettext('Invalid matcher expression'), e);
> +                return;
> +            }
> +
> +            function visit(obj) {
> +                for (const [key, value] of Object.entries(obj)) {
> +                    let ret;
> +
> +                    switch (key) {
> +                        case 'match':
> +                            {

this block...

> +                                switch (value.type) {
> +                                    case 'calendar':
> +                                        {

and these blocks are unnecessary. leaving them out reduces the 
indentation level here...

> +                                            ret = {
> +                                                type: 'match-calendar',
> +                                                data: {
> +                                                    value: value.schedule,
> +                                                },
> +                                                leaf: true,
> +                                                invert: false,
> +                                            };
> +                                        }
> +                                        break;
> +                                    case 'field':
> +                                        {
> +                                            ret = {
> +                                                type: 'match-field',
> +                                                data: {
> +                                                    field: value.field,
> +                                                },
> +                                                leaf: true,
> +                                                invert: false,
> +                                            };
> +
> +                                            if (value.regex) {
> +                                                ret.data.type = 'regex';
> +                                                ret.data.value = value.regex;
> +                                            } else {
> +                                                ret.data.type = 'exact';
> +                                                ret.data.value = value.values;
> +                                            }
> +                                        }
> +                                        break;
> +                                    case 'severity':
> +                                        {
> +                                            ret = {
> +                                                type: 'match-severity',
> +                                                data: {
> +                                                    value: value.severities,
> +                                                },
> +                                                leaf: true,
> +                                                invert: false,
> +                                            };
> +                                        }
> +                                        break;
> +                                }
> +                            }
> +                            break;
> +                        case 'all-of':
> +                        case 'any-of':
> +                        case 'one-of':
> +                            {
> +                                let children = [];
> +                                for (let child of value) {
> +                                    children.push(visit(child));
> +                                }
> +                                ret = {
> +                                    type: key,
> +                                    data: {},
> +                                    children,
> +                                    leaf: false,
> +                                    invert: false,
> +                                };
> +                            }
> +                            break;
> +                        case 'not':
> +                            {
> +                                let child = visit(value);
> +                                ret = {
> +                                    ...child,
> +                                    invert: !child.invert,
> +                                };
> +                            }
> +                            break;
> +                        case 'constant': {
> +                            ret = {
> +                                type: 'match-always',
> +                                data: {},
> +                                invert: !value,
> +                                leaf: true,
> +                            };
> +                        }
> +                    }
> +
> +                    let iconCls = me.getNodeIcon(
> +                        ret.type,
> +                        ret.data,
> +                        ret.invert,
> +                        !!ret.children?.length,
> +                    );
> +
> +                    ret.iconCls = iconCls;
> +                    if (!ret.leaf) {
> +                        ret.expanded = true;
> +                        ret.expandable = false;
> +                    }
> +
> +                    return ret;
> +                }
> +            }
> +
> +            let rootNode = visit(obj);
> +
> +            treeStore.setRootNode(rootNode);
> +        });
> +
> +        treeStore.addListener('datachanged', storeChanged);
> +
> +        let treePanel = Ext.create({
> +            xtype: 'treepanel',
> +            store: treeStore,
> +            // scope for the label wrapping rules in ext6-pmx.css
> +            cls: 'pmx-match-rule-tree',
> +            minHeight: 400,
> +            maxHeight: 400,
> +            scrollable: true,
> +            // wrapped labels have no predictable height, so render all rows
> +            // instead of estimating the scroll range from a sample row
> +            bufferedRenderer: false,
> +
> +            // Allow drag&drop for tree nodes
> +            viewConfig: {
> +                plugins: {
> +                    ptype: 'treeviewdragdrop',
> +                    containerScroll: true,
> +                },
> +            },
> +
> +            bind: {
> +                selection: '{selectedRecord}',
> +            },
> +
> +            columns: [
> +                {
> +                    xtype: 'treecolumn',
> +                    dataIndex: 'text',
> +                    renderer: me.renderTreeLabel,
> +                    flex: 1,
> +                    // rules can list many values, wrap them so that all of
> +                    // them stay visible; this implies variableRowHeight
> +                    cellWrap: true,
> +                },
> +                {
> +                    xtype: 'actioncolumn',
> +                    width: 60,
> +                    items: [
> +                        {
> +                            handler: (view, rI, cI, item, e, record) => {
> +                                let node = record.appendChild({
> +                                    type: 'match-field',
> +                                    data: {
> +                                        type: 'exact',
> +                                        field: '',
> +                                        value: '',
> +                                    },
> +                                    leaf: true,
> +                                });
> +
> +                                view.setSelection(node);
> +                            },
> +                            getTip: (v, m, rec) =>
> +                                Ext.String.format(gettext('Add new sub-rule'), v),
> +                            getClass: (v, m, { data }) => (data.leaf ? '' : 'fa fa-plus-circle'),
> +                            isActionDisabled: (v, r, c, i, rec) => rec.data.leaf,
> +                        },
> +                        {
> +                            handler: function (view, rI, cI, item, e, record) {
> +                                if (record.hasChildNodes()) {
> +                                    Ext.Msg.confirm(
> +                                        gettext('Remove rule?'),
> +                                        gettext('Remove rule and all of its sub-rules?'),
> +                                        (decision) => {
> +                                            if (decision === 'yes') {
> +                                                record.remove(true);
> +                                            }
> +                                        },
> +                                    );
> +                                } else {
> +                                    record.remove(true);
> +                                }
> +                            },
> +                            getTip: (v, m, rec) =>
> +                                Ext.String.format(gettext('Remove this rule'), v),
> +                            getClass: (v, m, { data }) => 'fa fa-trash-o',
> +                            isActionDisabled: (v, r, c, i, rec) => rec.isRoot(),
> +                        },
> +                    ],
> +                },
> +            ],
> +        });
> +
> +        Ext.apply(me, {
> +            items: [
> +                realExpression,
> +                treePanel,
> +                {
> +                    // nothing in the tree hints at the nodes being movable
> +                    xtype: 'container',
> +                    margin: '5 0 0 0',
> +                    html: gettext(
> +                        'Rules can be reordered and moved between groups with drag and drop.',
> +                    ),
> +                },
> +                errorLabel,
> +            ],
> +        });
> +        me.callParent();
> +    },
> +});
> +
> +Ext.define('Proxmox.panel.NotificationMatchExpressionNodePanel', {
> +    extend: 'Ext.panel.Panel',
> +    xtype: 'pmxNotificationMatchExpressionNodePanel',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +    border: false,
> +    layout: 'anchor',
> +
> +    items: [
> +        {
> +            xtype: 'proxmoxKVComboBox',
> +            fieldLabel: gettext('Rule Type'),
> +            isFormField: false,
> +            allowBlank: false,
> +            // Hide initially to avoid glitches when opening the window
> +            hidden: true,
> +            bind: {
> +                value: '{nodeType}',
> +                hidden: '{!showMatcherType}',
> +            },
> +
> +            comboItems: [
> +                ['match-always', gettext('Always match')],
> +                ['all-of', gettext('All sub-rules match')],
> +                ['any-of', gettext('At least one sub-rule matches')],
> +                ['one-of', gettext('Exactly one sub-rule matches')],
> +                ['match-field', gettext('Match Field')],
> +                ['match-severity', gettext('Match Severity')],
> +                ['match-calendar', gettext('Match Calendar')],
> +            ],
> +        },
> +        {
> +            xtype: 'proxmoxcheckbox',
> +            isFormField: false,
> +            fieldLabel: gettext('Invert Rule'),
> +            allowBlank: false,
> +            bind: {
> +                value: '{invertMatch}',
> +                hidden: '{!showMatcherType}',
> +            },
> +        },
> +        {
> +            xtype: 'component',
> +            height: 1,
> +            margin: '20 0 20 0',
> +            cls: 'pmx-horizontal-separator',
> +            bind: {
> +                hidden: '{!showMatcherType}',
> +            },
> +        },
> +        {
> +            xtype: 'pmxNotificationMatchFieldSettings',
> +            cbind: {
> +                baseUrl: '{baseUrl}',
> +            },
> +        },
> +        {
> +            xtype: 'pmxNotificationMatchSeveritySettings',
> +        },
> +        {
> +            xtype: 'pmxNotificationMatchCalendarSettings',
> +        },

I meant this here with the viewmodel.

instead of binding there, we could also do the hidden, etc. bind
here and have the immediate connection... (but that would be part of
a cleanup of the whole thing...)

> +    ],
> +});
> diff --git a/src/proxmox-dark/scss/extjs/_treepanel.scss b/src/proxmox-dark/scss/extjs/_treepanel.scss
> index 0480371..eb7d1cc 100644
> --- a/src/proxmox-dark/scss/extjs/_treepanel.scss
> +++ b/src/proxmox-dark/scss/extjs/_treepanel.scss
> @@ -22,3 +22,7 @@
>     background-color: $background-darker;
>     border-color: $border-color;
>   }
> +
> +.x-tree-node-text > code {
> +    background-color: $background-darkest;
> +}
> diff --git a/src/proxmox-dark/scss/proxmox/_general.scss b/src/proxmox-dark/scss/proxmox/_general.scss
> index dcbf049..36ff65c 100644
> --- a/src/proxmox-dark/scss/proxmox/_general.scss
> +++ b/src/proxmox-dark/scss/proxmox/_general.scss
> @@ -51,3 +51,7 @@ div.eol-notice + div[id^="panel-"] > div[id^="panel-"][id$="-bodyWrap"] > div {
>   .pmx-unclickable {
>     pointer-events: none;
>   }
> +
> +.pmx-horizontal-separator {
> +    border-top: 1px solid $border-color-alt;
> +}
> diff --git a/src/window/NotificationMatcherEdit.js b/src/window/NotificationMatcherEdit.js
> index 1999e20..893c3e3 100644
> --- a/src/window/NotificationMatcherEdit.js
> +++ b/src/window/NotificationMatcherEdit.js
> @@ -95,6 +95,9 @@ Ext.define('Proxmox.window.NotificationMatcherEdit', {
>               me.method = 'POST';
>           } else {
>               me.url += `/${me.name}`;
> +            if (Proxmox.Schema.notificationMatcherExpressions) {
> +                me.loadUrl = me.url + '?migrate-to-expression=1';
> +            }
>               me.method = 'PUT';
>           }
>   
> @@ -119,7 +122,7 @@ Ext.define('Proxmox.window.NotificationMatcherEdit', {
>                           {
>                               name: me.name,
>                               title: gettext('Match Rules'),
> -                            xtype: 'pmxNotificationMatchRulesEditPanel',
> +                            xtype: Proxmox.Schema.notificationMatcherPanelType(),
>                               isCreate: me.isCreate,
>                               baseUrl: me.baseUrl,
>                           },
> @@ -1013,7 +1016,7 @@ Ext.define('Proxmox.panel.MatchCalendarSettings', {
>       initComponent: function () {
>           let me = this;
>           Ext.apply(me.viewModel, {
> -            parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
> +            parent: me.up(Proxmox.Schema.notificationMatcherPanelType()).getViewModel(),
>           });
>           me.callParent();
>       },
> @@ -1091,7 +1094,7 @@ Ext.define('Proxmox.panel.MatchSeveritySettings', {
>       initComponent: function () {
>           let me = this;
>           Ext.apply(me.viewModel, {
> -            parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
> +            parent: me.up(Proxmox.Schema.notificationMatcherPanelType()).getViewModel(),
>           });
>           me.callParent();
>       },
> @@ -1164,7 +1167,7 @@ Ext.define('Proxmox.panel.MatchFieldSettings', {
>                               regexVal += `(${currentData.value.join('|')})`;
>                           }
>                           regexVal += '$';
> -                        newValue.push(regexVal);
> +                        newValue = regexVal;

doesn't that change the type from array to string now?
does that have any obvious effects on the older code?

>                       }
>   
>                       record.set({
> @@ -1282,7 +1285,7 @@ Ext.define('Proxmox.panel.MatchFieldSettings', {
>           });
>   
>           Ext.apply(me.viewModel, {
> -            parent: me.up('pmxNotificationMatchRulesEditPanel').getViewModel(),
> +            parent: me.up(Proxmox.Schema.notificationMatcherPanelType()).getViewModel(),
>           });
>           Ext.apply(me, {
>               items: [