Re: [PATCH manager v2 06/12] ui: ha: node affinity: move node priority selector into separate component

[David Riley <d.riley@proxmox.com>]

Thanks for sending in this patch.
comments inline.

On 6/2/26 12:02 PM, Daniel Kral wrote:
> PVE.form.NodePrioritySelector is a reduced adaption of
> PVE.form.VMSelector, which adds error highlights to the node priority
> selector and fetches the node list from the API directly.
>
> Signed-off-by: Daniel Kral<d.kral@proxmox.com>
> ---
> changes since v1:
> - new
>
>   www/manager6/Makefile                         |   1 +
>   www/manager6/ha/NodePrioritySelector.js       | 169 ++++++++++++++++++
>   www/manager6/ha/rules/NodeAffinityRuleEdit.js | 136 +-------------
>   3 files changed, 177 insertions(+), 129 deletions(-)
>   create mode 100644 www/manager6/ha/NodePrioritySelector.js
>
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index d4dd3f35..eb0e9d9c 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -151,6 +151,7 @@ JSSRC= 							\
>   	window/DirMapEdit.js				\
>   	window/GuestImport.js				\
>   	ha/Fencing.js					\
> +	ha/NodePrioritySelector.js			\
>   	ha/ResourceEdit.js				\
>   	ha/Resources.js					\
>   	ha/RuleEdit.js					\
> diff --git a/www/manager6/ha/NodePrioritySelector.js b/www/manager6/ha/NodePrioritySelector.js
> new file mode 100644
> index 00000000..ec6ac02a
> --- /dev/null
> +++ b/www/manager6/ha/NodePrioritySelector.js
> @@ -0,0 +1,169 @@
> +Ext.define('PVE.forms.NodePrioritySelector', {
> +    extend: 'Ext.grid.Panel',
> +    alias: 'widget.pveNodePrioritySelector',
> +
> +    mixins: {
> +        field: 'Ext.form.field.Field',
> +    },
> +
> +    allowBlank: true,
> +    selectAll: false,

Does this select all actually do anything here?
I can still select all nodes in the grid?

> +    isFormField: true,
> +
> +    store: {
> +        autoLoad: true,
> +        fields: ['node', 'cpu', 'mem', 'priority'],
> +        proxy: {
> +            type: 'proxmox',
> +            url: '/api2/json/nodes',
> +        },
> +        sorters: [
> +            {
> +                property: 'node',
> +                direction: 'ASC',
> +            },
> +        ],
> +    },
> +
> +    columns: [
> +        {
> +            header: gettext('Node'),
> +            flex: 1,
> +            dataIndex: 'node',
> +        },
> +        {
> +            header: gettext('Memory usage') + ' %',
> +            renderer: PVE.Utils.render_mem_usage_percent,
> +            sortable: true,
> +            width: 150,
> +            dataIndex: 'mem',
> +        },
> +        {
> +            header: gettext('CPU usage'),
> +            renderer: Proxmox.Utils.render_cpu,
> +            sortable: true,
> +            width: 150,
> +            dataIndex: 'cpu',
> +        },
> +        {
> +            header: gettext('Priority'),
> +            xtype: 'widgetcolumn',
> +            dataIndex: 'priority',
> +            sortable: true,
> +            stopSelection: true,
> +            widget: {
> +                xtype: 'proxmoxintegerfield',
> +                minValue: 0,
> +                maxValue: 1000,
> +                isFormField: false,
> +            },
> +        },
> +    ],
> +
> +    selModel: {
> +        selType: 'checkboxmodel',
> +        mode: 'SIMPLE',
> +    },
> +
> +    checkChangeEvents: ['selectionchange', 'change'],

Is this change event here even used?

> +
> +    listeners: {
> +        selectionchange: function () {
> +            // to trigger validity and error checks
> +            this.checkChange();
> +        },
> +    },
> +
> +    getSubmitData: function () {
> +        let me = this;
> +        let res = {};
> +        res[me.name] = me.getValue();
> +        return res;
> +    },
> +
> +    getValue: function () {
> +        let me = this;
> +
> +        if (me.savedValue !== undefined) {
> +            return me.savedValue;
> +        }
> +
> +        let sm = me.getSelectionModel();
> +        let selectedNodeModels = sm.getSelection() ?? [];
> +        let nodes = selectedNodeModels
> +            .map(({ data }) => data.node + (data.priority ? `:${data.priority}` : ''))
> +            .join(',');
> +
> +        return nodes;
> +    },
> +
> +    setValueSelection: function (value) {
> +        let me = this;
> +
> +        let store = me.getStore();
> +        let nodes = value.split(',').map((item) => {
> +            let [node, priority] = item.split(':');
> +
> +            let record = store.findRecord('node', node, 0, false, true, true);
> +            if (record) {
> +                record.set('priority', priority);
> +                record.commit();
> +            } else {
> +                let addedRecords = store.add({ node, priority });
> +                record = addedRecords[0];
> +            }
> +
> +            return record;
> +        });

Since this maps over a potentially long list of nodes and commits each
record individually, wrapping this block in store.beginUpdate() and
store.endUpdate() would prevent it from sending events for each set/add
operation. [0]

[0] https://docs.sencha.com/extjs/7.0.0/modern/Ext.data.Store.html#method-beginUpdate

> +
> +        let sm = me.getSelectionModel();
> +        if (nodes.length) {

I'm wondering if there is bug here.
If you call split on an empty string ("") using .split(',') it will return
[""], which will lead to nodes.length being 1 and therefore would
evaluate to true in this if. So it will call sm.select with [""].


> +            sm.select(nodes);
> +        } else {
> +            sm.deselectAll();
> +        }
> +
> +        me.getErrors();
> +    },
> +
> +    setValue: function (value) {
> +        let me = this;
> +
> +        let store = me.getStore();
> +        if (!store.isLoaded()) {
> +            me.savedValue = value;
> +            store.on(
> +                'load',
> +                function () {
> +                    me.setValueSelection(value);
> +                    delete me.savedValue;
> +                },
> +                { single: true },
> +            );
> +        } else {
> +            me.setValueSelection(value);
> +        }
> +
> +        return me.mixins.field.setValue.call(me, value);
> +    },
> +
> +    getErrors: function (value) {
> +        let me = this;
> +
> +        if (!me.isDisabled() && me.allowBlank === false && me.getValue().length === 0) {
> +            me.addBodyCls(['x-form-trigger-wrap-default', 'x-form-trigger-wrap-invalid']);
> +            return [gettext('No nodes selected')];

Not sure if this returned text is visible in the UI. I played around with it and the grid turned
red if no node is selected, but due to the fact that the "Add" button is disabled anyways this
message will never appear, but it does not hurt either.

> +        }
> +
> +        me.removeBodyCls(['x-form-trigger-wrap-default', 'x-form-trigger-wrap-invalid']);
> +
> +        return [];
> +    },


Not sure if I like this approach. I tried to look for better solutions but it seems like
there is no extjs native way to handle this.

> +
> +    initComponent: function () {
> +        let me = this;
> +
> +        me.callParent();
> +        me.initField();
> +    },
> +});
> diff --git a/www/manager6/ha/rules/NodeAffinityRuleEdit.js b/www/manager6/ha/rules/NodeAffinityRuleEdit.js
> index b4b6a13c..77da18b1 100644
> --- a/www/manager6/ha/rules/NodeAffinityRuleEdit.js
> +++ b/www/manager6/ha/rules/NodeAffinityRuleEdit.js
> @@ -4,133 +4,6 @@ Ext.define('PVE.ha.rules.NodeAffinityInputPanel', {
>       initComponent: function () {
>           let me = this;
>   
> -        /* TODO Node selector should be factored out in its own component */
> -        let update_nodefield, update_node_selection;
> -
> -        let sm = Ext.create('Ext.selection.CheckboxModel', {
> -            mode: 'SIMPLE',
> -            listeners: {
> -                selectionchange: function (model, selected) {
> -                    update_nodefield(selected);
> -                },
> -            },
> -        });
> -
> -        let store = Ext.create('Ext.data.Store', {
> -            fields: ['node', 'mem', 'cpu', 'priority'],
> -            data: PVE.data.ResourceStore.getNodes(), // use already cached data to avoid an API call
> -            proxy: {
> -                type: 'memory',
> -                reader: { type: 'json' },
> -            },
> -            sorters: [
> -                {
> -                    property: 'node',
> -                    direction: 'ASC',
> -                },
> -            ],
> -        });
> -
> -        var nodegrid = Ext.createWidget('grid', {
> -            store: store,
> -            border: true,
> -            height: 300,
> -            selModel: sm,
> -            columns: [
> -                {
> -                    header: gettext('Node'),
> -                    flex: 1,
> -                    dataIndex: 'node',
> -                },
> -                {
> -                    header: gettext('Memory usage') + ' %',
> -                    renderer: PVE.Utils.render_mem_usage_percent,
> -                    sortable: true,
> -                    width: 150,
> -                    dataIndex: 'mem',
> -                },
> -                {
> -                    header: gettext('CPU usage'),
> -                    renderer: Proxmox.Utils.render_cpu,
> -                    sortable: true,
> -                    width: 150,
> -                    dataIndex: 'cpu',
> -                },
> -                {
> -                    header: gettext('Priority'),
> -                    xtype: 'widgetcolumn',
> -                    dataIndex: 'priority',
> -                    sortable: true,
> -                    stopSelection: true,
> -                    widget: {
> -                        xtype: 'proxmoxintegerfield',
> -                        minValue: 0,
> -                        maxValue: 1000,
> -                        isFormField: false,
> -                        listeners: {
> -                            change: function (numberfield, value, old_value) {
> -                                let record = numberfield.getWidgetRecord();
> -                                record.set('priority', value);
> -                                update_nodefield(sm.getSelection());
> -                                record.commit();
> -                            },
> -                        },
> -                    },
> -                },
> -            ],
> -        });
> -
> -        let nodefield = Ext.create('Ext.form.field.Hidden', {
> -            name: 'nodes',
> -            value: '',
> -            listeners: {
> -                change: function (field, value) {
> -                    update_node_selection(value);
> -                },
> -            },
> -            isValid: function () {
> -                let value = this.getValue();
> -                return value && value.length !== 0;
> -            },
> -        });
> -
> -        update_node_selection = function (string) {
> -            let nodes = string.split(',').map((item) => {
> -                let [node, priority] = item.split(':');
> -
> -                let record = store.findRecord('node', node, 0, false, true, true);
> -                if (record) {
> -                    record.set('priority', priority);
> -                    record.commit();
> -                } else {
> -                    let addedRecords = store.add({ node, priority });
> -                    record = addedRecords[0];
> -                }
> -
> -                return record;
> -            });
> -
> -            if (nodes.length) {
> -                sm.select(nodes);
> -            } else {
> -                sm.deselectAll();
> -            }
> -
> -            nodegrid.reconfigure(store);
> -        };
> -
> -        update_nodefield = function (selected) {
> -            let nodes = selected
> -                .map(({ data }) => data.node + (data.priority ? `:${data.priority}` : ''))
> -                .join(',');
> -
> -            // nodefield change listener calls us again, which results in a
> -            // endless recursion, suspend the event temporary to avoid this
> -            nodefield.suspendEvent('change');
> -            nodefield.setValue(nodes);
> -            nodefield.resumeEvent('change');
> -        };
> -
>           me.column2 = [
>               {
>                   xtype: 'proxmoxcheckbox',
> @@ -145,10 +18,15 @@ Ext.define('PVE.ha.rules.NodeAffinityInputPanel', {
>                   uncheckedValue: 0,
>                   defaultValue: 0,
>               },
> -            nodefield,
>           ];
>   
> -        me.columnB = [nodegrid];
> +        me.columnB = [
> +            {
> +                xtype: 'pveNodePrioritySelector',
> +                name: 'nodes',
> +                allowBlank: false,
> +            },
> +        ];
>   
>           me.callParent();
>       },