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

["Daniel Kral" <d.kral@proxmox.com>]

On Mon Jul 13, 2026 at 2:43 PM CEST, David Riley wrote:
> Thanks for sending in this patch.
> comments inline.
>
> On 6/2/26 12:02 PM, Daniel Kral wrote:

[ snip ]

>> 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?
>

Good question, to be honest, I aligned this with the existing
PVE.form.VMSelector and similar extensions + mixins from Ext.grid.Panel
and Ext.form.field.Field...

At least for ExtJS 7.0.0 I cannot find anywhere in our codebase nor the
ExtJS codebase where this property is some flag. It only is used as a
class method in some components.

Will remove that in the next revision and might send a patch which
removes the same property from the other components as well. Haven't
checked though whether this had any functionality in past versions of
ExtJS.

>> +    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?
>

You're right, doesn't seem like it... I'll remove it if I don't see any
reason to leave it here.

>> +
>> +    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
>

Nice, thanks for that!

Though we don't expect the nodelist to become any longer than 48 nodes
as of now, it's good to have this in place already, will use it in the
next revision!

>> +
>> +        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 [""].
>
>

Yeah, the whole existing code section assumes that the API never outputs
an empty string... Though this would kind of break already at the
splitting of the node items then.

This should not happen. I could slip in a preceding patch, which
filter()s it away, but my gut feeling tells me that it shouldn't ignore
wrong API output as it would make it harder to spot that there's
something wrong there... I'll think about it.

>> +            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.
>

Yeah, this was also taken from the VMSelector component... The nodes
field is required here and probably will always be for the node priority
selector.

>> +        }
>> +
>> +        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.
>

What approach are you referring to exactly?

>> +
>> +    initComponent: function () {
>> +        let me = this;
>> +
>> +        me.callParent();
>> +        me.initField();
>> +    },
>> +});

[ snip ]