Some comments inline - did the review mostly in tandem with testing the
UI, to get a better context.

On Fri Mar 28, 2025 at 6:13 PM CET, Gabriel Goller wrote:
[..]
> diff --git a/www/manager6/dc/Config.js b/www/manager6/dc/Config.js
> index 74728c8320e9..68f7be8d6042 100644
> --- a/www/manager6/dc/Config.js
> +++ b/www/manager6/dc/Config.js
> @@ -229,6 +229,14 @@ Ext.define('PVE.dc.Config', {
>                   hidden: true,
>                   iconCls: 'fa fa-shield',
>                   itemId: 'sdnfirewall',
> +             },
> +             {
> +                 xtype: 'pveSDNFabricView',
> +                 groups: ['sdn'],
> +                 title: gettext('Fabrics'),

Do we really want to translate 'Fabrics'? Or rather just keep the name
always at 'Fabrics' as the name of the concept/feature itself? Since it
is a established term, after all. I'd keep it, IMHO.

(But we seem to translate everything else here in the file, so really
not sure.)

> +                 hidden: true,
> +                 iconCls: 'fa fa-road',
> +                 itemId: 'sdnfabrics',
>               });
>           }
>
> diff --git a/www/manager6/sdn/FabricsView.js b/www/manager6/sdn/FabricsView.js
> new file mode 100644
> index 000000000000..0ef12defb1a8
> --- /dev/null
> +++ b/www/manager6/sdn/FabricsView.js
[..]
> +Ext.define('PVE.sdn.Fabric.View', {
[..]
> +     {
> +         text: gettext('Protocol'),
> +         dataIndex: 'protocol',
> +         width: 100,
> +         renderer: function(value, metaData, rec) {
> +             if (rec.data.type !== 'fabric') {
> +                 return "";
> +             }
> +
> +             return PVE.Utils.render_sdn_pending(rec, value, 'protocol');
> +         },

A mapping for the internal protocol name to a user-visible would be nice
here, i.e. 'ospf' -> 'OSPF' and 'openfabric' -> 'OpenFabric'. Not much
difference currently, but would look a bit better in the UI :)

> +     },
> +     {
> +         text: gettext('Loopback IP'),
> +         dataIndex: 'router_id',
> +         width: 150,
> +         renderer: function(value, metaData, rec) {
> +             if (rec.data.type === 'fabric') {
> +                 return PVE.Utils.render_sdn_pending(rec, 
> rec.data.loopback_prefix, 'loopback_prefix');
> +             }
> +
> +             return PVE.Utils.render_sdn_pending(rec, value, 'router_id');
> +         },
> +     },
> +     {
> +         text: gettext('Action'),
> +         xtype: 'actioncolumn',
> +         dataIndex: 'text',
> +         width: 100,
> +         items: [
> +             {
> +                 handler: 'addActionTreeColumn',
> +                 getTip: (_v, _m, _rec) => gettext('Add'),

nit: "Add Node" would be better here for node rows, if that's possible.

> +                 getClass: (_v, _m, { data }) => {
> +                     if (data.type === 'fabric') {
> +                         return 'fa fa-plus-circle';
> +                     }
> +
> +                     return 'pmx-hidden';
> +                 },
> +                 isActionDisabled: (_v, _r, _c, _i, { data }) => data.type 
> !== 'fabric',
> +             },
> +             {
> +                 tooltip: gettext('Edit'),

^ same as above

> +                 handler: 'editAction',
> +                 getClass: (_v, _m, { data }) => {
> +                     // the fabric type (openfabric, ospf, etc.) cannot be 
> edited
> +                     if (data.type) {
> +                         return 'fa fa-pencil fa-fw';
> +                     }
> +
> +                     return 'pmx-hidden';
> +                 },
> +                 isActionDisabled: (_v, _r, _c, _i, { data }) => !data.type,
> +             },
> +             {
> +                 tooltip: gettext('Delete'),

^ and same here as well

> +                 handler: 'deleteAction',
> +                 getClass: (_v, _m, { data }) => {
> +                     // the fabric type (openfabric, ospf, etc.) cannot be 
> deleted
> +                     if (data.type) {
> +                         return 'fa critical fa-trash-o';
> +                     }
> +
> +                     return 'pmx-hidden';
> +                 },
> +                 isActionDisabled: (_v, _r, _c, _i, { data }) => !data.type,
> +             },
> +         ],
> +     },
> +     {
> +         header: gettext('Interfaces'),
> +         width: 100,
> +         dataIndex: 'interface',
> +         renderer: function(value, metaData, rec) {
> +             let interfaces = rec.data.pending?.interface || 
> rec.data.interface || [];

nit: could be const (see [0] tho regarding rules for const/let)

[0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables

> +
> +             let names = interfaces.map((iface) => {
> +                 let properties = Proxmox.Utils.parsePropertyString(iface);

nit: same here, as well for some other things in this file

> +                 return properties.name;
> +             });
> +
> +             names.sort();
> +             return names.join(", ");
> +         },
> +     },
[..]
> +
> +    initComponent: function() {
> +     let me = this;
> +
> +
> +     let add_button = new Proxmox.button.Button({

New variables should generally be camelCase [0].

[1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

> +         text: gettext('Add Node'),
> +         handler: 'addActionTbar',
> +         disabled: true,
> +     });

Since there is also an "add node" button in the "Action" column for each
fabric, do we need a separate one in the top-bar as well?

> +
> +     let set_add_button_status = function() {
> +         let selection = me.view.getSelection();
> +
> +         if (selection.length === 0) {
> +             return;
> +         }
> +
> +         add_button.setDisabled(selection[0].data.type !== 'fabric');
> +     };
> +
> +     Ext.apply(me, {
> +         tbar: [
> +             {
> +                 text: gettext('Add Fabric'),
> +                 menu: [
> +                     {
> +                         text: gettext('OpenFabric'),

'OpenFabric' also should not be translated, right? As its the name of
the implementation/protocol.

> +                         handler: 'openAddOpenFabricWindow',
> +                     },
> +                     {
> +                         text: gettext('OSPF'),

^ same here

> +                         handler: 'openAddOspfWindow',
> +                     },
> +                 ],
> +             },
[..]
> +    controller: {
> +     xclass: 'Ext.app.ViewController',
> +
> +     reload: function() {
> +         let me = this;
> +
> +         Proxmox.Utils.API2Request({
> +             url: `/cluster/sdn/fabrics/?pending=1`,
> +             method: 'GET',
> +             success: function(response, opts) {
> +                 let ospf = response.result.data.ospf;
> +                 let openfabric = response.result.data.openfabric;
> +
> +                 // add some metadata so we can merge the objects later and 
> still know the protocol/type
> +                 ospf = ospf.map(x => {
> +                     if (x.state && x.state === 'new') {
> +                         Object.assign(x, x.pending);
> +                     }
> +
> +                     if (x.ty === 'fabric') {
> +                         return Object.assign(x, { protocol: "ospf", name: 
> x.area });
> +                     } else if (x.ty === 'node') {
> +                         let id = x.node_id.split("_");
> +                         return Object.assign(x,
> +                             {
> +                                 protocol: "ospf",
> +                                 node: id[1],
> +                                 fabric: id[0],
> +                             },
> +                         );
> +                     } else {
> +                         return x;
> +                     }
> +                 });

Perhaps factor this out a bit into a separate function, making the
success() definition here a bit more succinct?

> +                 openfabric = openfabric.map(x => {
> +                     if (x.state && x.state === 'new') {
> +                         Object.assign(x, x.pending);
> +                     }
> +
> +                     if (x.ty === 'fabric') {
> +                         return Object.assign(x, { protocol: "openfabric", 
> name: x.fabric_id });
> +                     } else if (x.ty === 'node') {
> +                         let id = x.node_id.split("_");
> +                         return Object.assign(x,
> +                             {
> +                                 protocol: "openfabric",
> +                                 node: id[1],
> +                                 fabric: id[0],
> +                             },
> +                         );
> +                     } else {
> +                         return x;
> +                     }
> +                 });

^ same for the openfabric mapping

> +
> +                 let allFabrics = openfabric.concat(ospf);
> +                 let fabrics = allFabrics.filter(e => e.ty === 
> "fabric").map((fabric) => {
> +                     if (!fabric.state || fabric.state !== 'deleted') {
> +                         fabric.children = allFabrics.filter(e => e.ty === 
> "node")
> +                             .filter((node) =>
> +                                 node.fabric === fabric.name && 
> node.protocol === fabric.protocol)
> +                                     .map((node) => {
> +                                         Object.assign(node, {
> +                                             leaf: true,
> +                                             type: 'node',
> +                                             iconCls: 'fa fa-desktop 
> x-fa-treepanel',
> +                                             name: node.node,
> +                                             _fabric: fabric.name,
> +                                         });
> +
> +                                         return node;
> +                                     });

^ and same here, moving it into a separate function instead of
open-coding it here. Would make the whole bit here more readable/easier
to understand.

> +                     }
> +
> +                     Object.assign(fabric, {
> +                         type: 'fabric',
> +                         protocol: fabric.protocol,
> +                         expanded: true,
> +                         name: fabric.fabric_id || fabric.area,
> +                         iconCls: 'fa fa-road x-fa-treepanel',
> +                     });
> +
> +                     return fabric;
> +                 });
> +
[..]
> +
> +     addActionTreeColumn: function(_grid, _rI, _cI, _item, _e, rec) {
> +         let me = this;

nit: never used, just drop it

> +         this.addAction(rec);
> +     },
> +
[..]
> +
> +     deleteAction: function(table, rI, cI, item, e, { data }) {
> +         let me = this;
> +         let view = me.getView();
> +
> +         Ext.Msg.show({
> +             title: gettext('Confirm'),
> +             icon: Ext.Msg.WARNING,
> +             message: Ext.String.format(gettext('Are you sure you want to 
> remove the fabric {0}?'), `${data.name}`),

nit: unnecessary string interpolation

> +             buttons: Ext.Msg.YESNO,
> +             defaultFocus: 'no',
> +             callback: function(btn) {
> +                 if (btn !== 'yes') {
> +                     return;
> +                 }
> +
> +                 let url;
> +                 if (data.type === "node") {
> +                     url = 
> `/cluster/sdn/fabrics/${data.protocol}/${data._fabric}/node/${data.name}`;
> +                 } else if (data.type === "fabric") {
> +                     url = 
> `/cluster/sdn/fabrics/${data.protocol}/${data.name}`;
> +                 } else {
> +                     console.warn("deleteAction: missing type");
> +                 }
> +
> +                 Proxmox.Utils.API2Request({
> +                     url,
> +                     method: 'DELETE',
> +                     waitMsgTarget: view,
> +                     failure: function(response, opts) {
> +                         Ext.Msg.alert(gettext('Error'), 
> response.htmlStatus);

Could use `Proxmox.Utils.errorText` here instead of `gettext('Error')`.

> +                     },
> +                     callback: me.reload.bind(me),

nit: `() => me.reload()` is already used multiple times in this file, so
use that here too for consistency? Think it's the preferred style
anyway.

> +                 });
> +             },
> +         });
> +     },
> +
> +     openAddOpenFabricWindow: function() {
> +         let me = this;
> +
> +         let window = Ext.create('PVE.sdn.Fabric.OpenFabric.Fabric.Edit', {
> +             autoShow: true,
> +             autoLoad: false,
> +             isCreate: true,
> +         });
> +
> +         window.on('destroy', () => me.reload());
> +     },
> +
> +     openAddOspfWindow: function() {
> +         let me = this;
> +
> +         let window = Ext.create('PVE.sdn.Fabric.Ospf.Fabric.Edit', {
> +             autoShow: true,
> +             autoLoad: false,
> +             isCreate: true,
> +         });
> +
> +         window.on('destroy', () => me.reload());
> +     },
> +
> +     init: function(view) {
> +         let me = this;
> +         me.reload();
> +     },
> +    },
> +});



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Reply via email to