consoles: Redesign

[mvollmer]

COCKPIT-1225

• Design: Refresh design of VM consoles #2074

Demo: https://www.youtube.com/watch?v=PHHoSxs70Ho

Questions:

• Where should the "send key" and "disconnect" actions go?
• Is the (?) button plus popover ok?
• Is it ok to completely ignore spice when there is vnc?
• How should the expanded view look?

[mvollmer]

We should build this all up from scratch without react-console. This means writing our own NoVNC React wrapper. That's very feasible and worth it, IMO, but can be done separately.

The move to PF6 makes this more important. React-console 6.0.0 uses inline CSS and generated class names, both of which is problematic for us.

[sourcery-ai]

Hey @mvollmer - I've reviewed your changes - here's some feedback:

Overall Comments:

• This is a large change, so consider breaking it up into smaller, more manageable pull requests.
• The diff includes a lot of refactoring and moving code around, which makes it hard to review; consider separating functional changes from pure refactoring.

Here's what I looked at during the review

• 🟡 General issues: 1 issue found
• 🟢 Security: all looks good
• 🟢 Testing: all looks good
• 🟡 Complexity: 2 issues found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion: Review the parameters and naming of domainModifyXML.

Since this function generalizes XML modifications for both adding and editing devices, consider whether its parameter order or naming might be improved for clarity. For instance, documenting the expected values for 'action' and 'type' would help maintainers safely extend this function.

Suggested change

	async function domainModifyXML(vm, action, option, type, values) {
	/**
	* Modifies XML for Libvirt domains.
	*
	* @param {Object} vm - The virtual machine connection object.
	* @param {string} action - The XML modification action. Expected values: "add", "edit", "delete".
	* @param {string} option - The XML option to be modified.
	* @param {string} type - The device type. Expected values include "disk", "interface", etc.
	* @param {Object} values - Key-value pairs representing the new settings.
	*/
	async function domainModifyXML(vm, action, option, type, values) {

[sourcery-ai]

issue (complexity): Consider extracting portions of the render logic into smaller, dedicated components or helper functions to improve clarity and maintainability, such as extracting the console body selection and tab generation into separate functions/components .

The overall changes add more UI details and branch handling, but the complexity in rendering logic (with deep nested conditions for each console type) remains high. To keep functionality intact while improving clarity and maintainability, consider extracting portions of the render logic into small, dedicated components or helper functions. For example, you might extract the console body selection and tab generation into separate functions/components. This not only makes the main render method leaner but also makes each part easier to test and update independently.

Actionable Steps:

1. Extract Console Body Logic:
   Create a helper function/component (e.g. ConsoleBody) that takes in state, vm, onAddErrorNotification, and isExpanded and returns the appropriate body JSX.

   // ConsoleBody.jsx
   import React from 'react';
   import { VncMissing, VncInactive, VncActive, VncActiveActions, VncPending } from './vnc';
   import { SpiceActive, SpiceInactive } from './spice';
   import { SerialActive, SerialActiveActions, SerialInactive, SerialPending, SerialMissing } from './serial';
   
   export const ConsoleBody = ({ state, vm, onAddErrorNotification, isExpanded }) => {
     const serials = vm.displays && vm.displays.filter(disp => disp.type === 'pty');
     const inactive_serials = vm.inactiveXML.displays && vm.inactiveXML.displays.filter(disp => disp.type === 'pty');
     const vnc = vm.displays && vm.displays.find(disp => disp.type === 'vnc');
     const inactive_vnc = vm.inactiveXML.displays && vm.inactiveXML.displays.find(disp => disp.type === 'vnc');
     const spice = vm.displays && vm.displays.find(disp => disp.type === 'spice');
     const inactive_spice = vm.inactiveXML.displays && vm.inactiveXML.displays.find(disp => disp.type === 'spice');
   
     const type = state.type || (vnc || serials.length === 0 ? "graphical" : "text0");
   
     if (type === "graphical") {
       if (vm.state !== "running") {
         if (!inactive_vnc && !inactive_spice) {
           return <VncMissing vm={vm} />;
         } else if (inactive_vnc) {
           return <VncInactive vm={vm} inactive_vnc={inactive_vnc} onAddErrorNotification={onAddErrorNotification} />;
         }
         return <SpiceInactive vm={vm} />;
       }
   
       if (vnc) {
         return (
           <>
             <VncActive state={state.vncState} vm={vm} consoleDetail={vnc} inactiveConsoleDetail={inactive_vnc} spiceDetail={spice} onAddErrorNotification={onAddErrorNotification} isExpanded={isExpanded} />
             <VncActiveActions state={state.vncState} vm={vm} vnc={vnc} />
           </>
         );
       } else if (inactive_vnc) {
         return <VncPending vm={vm} inactive_vnc={inactive_vnc} onAddErrorNotification={onAddErrorNotification} />;
       } else if (spice) {
         return <SpiceActive vm={vm} spice={spice} />;
       } else {
         return <VncMissing vm={vm} onAddErrorNotification={onAddErrorNotification} />;
       }
     }
   
     // Handle serial / text consoles.
     if (serials.length > 0) {
       const serial_state = state.serialStates.get(serials[0].alias || 0);
       if (vm.state !== "running") {
         return <SerialInactive vm={vm} />;
       }
       return (
         <>
           <SerialActive state={serial_state} connectionName={vm.connectionName} vmName={vm.name} spawnArgs={/* your spawnArgs */} />
           <SerialActiveActions state={serial_state} />
         </>
       );
     } else {
       if (inactive_serials.length > 0) {
         return <SerialPending vm={vm} />;
       }
       return <SerialMissing vm={vm} onAddErrorNotification={onAddErrorNotification} />;
     }
   };
   
   export default ConsoleBody;

2. Extract Tabs Generation:
   Similarly, you can create a helper component (e.g. ConsoleTabs) that generates the toggle group items based on the console types available.

   // ConsoleTabs.jsx
   import React from 'react';
   import { ToggleGroup, ToggleGroupItem } from '@patternfly/react-core';
   
   const ConsoleTabs = ({ vm, state }) => {
     const serials = vm.displays && vm.displays.filter(display => display.type === 'pty');
     const vnc = vm.displays && vm.displays.find(display => display.type === 'vnc');
     const spice = vm.displays && vm.displays.find(display => display.type === 'spice');
   
     let type = state.type || (vnc || serials.length === 0 ? 'graphical' : 'text0');
     const tabs = [];
   
     tabs.push(
       <ToggleGroupItem
         key="graphical"
         text="Graphical"
         isSelected={type === 'graphical'}
         onChange={() => state.setType('graphical')}
       />
     );
   
     if (serials.length > 0) {
       serials.forEach((pty, idx) => {
         const key = "text" + idx;
         tabs.push(
           <ToggleGroupItem
             key={key}
             text={serials.length === 1 ? "Text" : `Text (${pty.alias || idx})`}
             isSelected={type === key}
             onChange={() => state.setType(key)}
           />
         );
       });
     } else {
       tabs.push(
         <ToggleGroupItem
           key="text0"
           text="Text"
           isSelected={type === 'text0'}
           onChange={() => state.setType('text0')}
         />
       );
     }
   
     return <ToggleGroup>{tabs}</ToggleGroup>;
   };
   
   export default ConsoleTabs;

3. Simplify Main Component (ConsoleCard):
   With these components, the main component becomes easier to parse.

   // ConsoleCard.jsx
   import React from 'react';
   import { Card, CardBody, CardHeader, CardTitle } from '@patternfly/react-core';
   import { Split, SplitItem } from '@patternfly/react-core/dist/esm/layouts/Split';
   import { ExpandIcon, CompressIcon } from '@patternfly/react-icons';
   import { Button } from "@patternfly/react-core";
   import ConsoleBody from './ConsoleBody';
   import ConsoleTabs from './ConsoleTabs';
   import { vmId } from "../../../helpers.js";
   
   export const ConsoleCard = ({ state, vm, config, onAddErrorNotification, isExpanded }) => {
     const actions = [];
   
     if (!isExpanded) {
       actions.push(
         <Button
           key="expand"
           variant="link"
           onClick={() => {
             const urlOptions = { name: vm.name, connection: vm.connectionName };
             return cockpit.location.go(["vm", "console"], { ...cockpit.location.options, ...urlOptions });
           }}
           icon={<ExpandIcon />}
           iconPosition="right">Expand</Button>
       );
     } else {
       actions.push(
         <Button
           key="compress"
           variant="link"
           onClick={() => {
             const urlOptions = { name: vm.name, connection: vm.connectionName };
             return cockpit.location.go(["vm"], { ...cockpit.location.options, ...urlOptions });
           }}
           icon={<CompressIcon />}
           iconPosition="right">Compress</Button>
       );
     }
   
     return (
       <Card isPlain={isExpanded} className="ct-card consoles-card" id={`${vmId(vm.name)}-consoles`}>
         <CardHeader actions={{ actions }}>
           <Split hasGutter>
             <SplitItem>
               <CardTitle component="h2">{isExpanded ? vm.name : "Console"}</CardTitle>
             </SplitItem>
             <SplitItem>
               <ConsoleTabs vm={vm} state={state} />
             </SplitItem>
           </Split>
         </CardHeader>
         <CardBody>
           <div className="vm-console">
             <ConsoleBody state={state} vm={vm} onAddErrorNotification={onAddErrorNotification} isExpanded={isExpanded} />
           </div>
         </CardBody>
       </Card>
     );
   };

By extracting these pieces, you reduce the cognitive load within a single component and ease future maintenance or enhancements.

[sourcery-ai]

issue (complexity): Consider simplifying state management by using React's built-in hooks instead of an EventEmitter and custom event wiring when dynamic event wiring isn't needed, as this reduces abstraction and boilerplate while retaining dynamic update ability via a setState callback wrapper passed to state objects, encapsulated in a custom hook like useSimpleState to keep behavior transparent and focused, replacing the need for EventEmitter and follow methods with a standard state update mechanism.

If you don’t need full event-based plumbing for most state updates – especially for simple state objects – you could consider simplifying by relying more on React’s built-in hooks. For example, instead of embedding an EventEmitter inside your state object and then wiring in “render” events, consider a custom hook that couples useState with an update call:

// Before (custom abstraction)
class StateObject extends EventEmitter {
  update() {
    this.emit("render");
  }
  follow(obj, callback) {
    return obj.on("render", callback || (() => this.update()));
  }
  close() {}
}

export function useStateObject(constructor, deps, comps) {
  const state = useObject(constructor, obj => obj.close(), deps || [], comps);
  useOn(state, "render");
  return state;
}

// After (simpler approach using built-in React hooks)
function useSimpleState(constructor) {
  const [state, setState] = useState(constructor());

  // Assume your state objects have methods that should trigger a re-render;
  // you can wrap them to automatically update local state.
  const update = useCallback(() => setState({ ...state }), [state]);
  // Inject this updater into your state if needed:
  if (typeof state.setUpdate === 'function') {
    state.setUpdate(update);
  }
  return state;
}

Example usage:
class MyState {
  constructor() {
    this.whatever = 12;
  }
  // Allow a callback to be set that triggers a re-render.
  setUpdate(cb) {
    this._update = cb;
  }
  setWhatever(val) {
    this.whatever = val;
    if (this._update) {
      this._update();
    }
  }
}

const MyComp = () => {
  const state = useSimpleState(() => new MyState());
  return <span>{state.whatever}</span>;
};

Actionable Steps:

1. Remove EventEmitter and follow: If dynamic event wiring isn’t critical, substitute it with a standard state update mechanism (e.g. reassigning state with useState).
2. Provide an updating callback: Instead of using an event “render” to trigger re-renders, pass a callback (like setState wrapper) to your state objects.
3. Encapsulate logic in a small custom hook: Use a focused hook (as in useSimpleState) to minimize boilerplate while keeping behavior transparent.

This retains your dynamic update ability while reducing the extra layer of abstraction introduced by EventEmitter and follow methods.

[cockpituous]

This added line is not executed by any test.

[cockpituous]

This added line is not executed by any test.

[cockpituous]

This added line is not executed by any test.

[cockpituous]

These 2 added lines are not executed by any test.

[cockpituous]

This added line is not executed by any test.

[cockpituous]

This added line is not executed by any test.

[cockpituous]

These 4 added lines are not executed by any test.

[cockpituous]

This added line is not executed by any test.

[cockpituous]

This added line is not executed by any test.

[cockpituous]

These 4 added lines are not executed by any test.