VariantManagament: breaks when array of variants is empty

[superdyzio]

Describe the bug

Initially when we load our views we have empty variants array so there is no <VariantItem> children and also selectedVariant = ''. In this case our app is breaking because of the issue here:

// packages/main/src/components/VariantManagement/index.tsx
const showSaveBtn = dirtyState && !Object.prototype.hasOwnProperty.call(selectedVariant, 'readOnly');

Isolated Example

No response

Reproduction steps

No response

Expected Behaviour

Children property is typed as ReactNode | ReactNode[] so it should not break with empty array.

Screenshots or Videos

No response

UI5 Web Components for React Version

2.4.0

UI5 Web Components Version

2.4.0

Browser

Chrome

Operating System

MacOS Sequoia 15.1

Additional Context

No response

Relevant log output

No response

Organization

SAP Fioneer

Declaration

• I’m not disclosing any internal or sensitive information.

[superdyzio]

I will be happy to provide a one-liner PR to prevent this issue from happening 🙆‍♂️

[Lukas742]

Hi @superdyzio

thank you for the offer, but let me understand the issue first.
Are you saying that the VariantManagement breaks if empty children are passed?
I've tried reproducing this issue using most values that lead to empty children, but the component seems to be rendering fine.
Only values that are rendered in the DOM and which are not supported will break the component (e.g. string, number, all components that aren't a VariantItem). This is the current state for most components that expect specific components as children, hence the note:

Note: Although this prop accepts all HTML Elements, it is strongly recommended that you only use VariantItem in order to preserve the intended design.

Here you can find the example with different values leading to empty children:
https://stackblitz.com/edit/ui5wcr-vm-saveas-ngdw14?file=src%2FApp.tsx

[superdyzio]

we have this code providing the <VariantItem> list:

const renderedVariants = useMemo(() => {
    return variants.map((item) => {
      return (
        <UiVariantItem
          key={item.name}
          global={item.global}
          readOnly={item.readOnly}
          labelReadOnly={item.labelReadOnly}
          favorite={item.favorite}
          isDefault={defaultView === item.name}
          selected={selectedView === item.name}
        >
          {item.name}
        </UiVariantItem>
      );
    });
  }, [variants]);

UiVariantItem is just a simplest wrapper:

export const UiVariantItem = ({
  children,
  favorite,
  isDefault,
  readOnly,
  selected,
  labelReadOnly,
}: Props) => (
  <VariantItem
    favorite={favorite}
    labelReadOnly={labelReadOnly}
    isDefault={isDefault}
    readOnly={readOnly}
    selected={selected}
  >
    {children}
  </VariantItem>
);

when the variants array is empty (which happens for a brief period during our components initialisation) we get an error:
TypeError: Cannot convert undefined or null to object at hasOwnProperty (<anonymous>)
coming from line:
const showSaveBtn = dirtyState && !Object.prototype.hasOwnProperty.call(selectedVariant, 'readOnly');
because selectedVariant is undefined at the moment

in my opinion it's reasonable to make this resistant to such case by adding one condition:
const showSaveBtn = dirtyState && selectedVariant && !Object.prototype.hasOwnProperty.call(selectedVariant, 'readOnly');

[Lukas742]

What I'm curious about is why it's breaking for you, because the provided code snippet should work without throwing an error: https://stackblitz.com/edit/ui5wcr-vm-saveas-ucervk?file=src%2FApp.tsx

Please take a look at the StackBlitz example above and edit it in case I missed something.

[superdyzio]

@Lukas742 I can break it by adding dirtyState={true}, it makes sense looking at the code 😄 in our solution we initialise with empty array of variants and also we have this:

const isDirty = useMemo(() => {
    const cleanedSelectedViewSettings = removeLabelsFromSettings(
      getViewByName(selectedView)?.settings,
    );
    const cleanedCurrentSettings = removeLabelsFromSettings(settings);

    return !isEqual(cleanedSelectedViewSettings, cleanedCurrentSettings);
  }, [settings, selectedView]);

in the first execution cleanedSelectedViewSettings is undefined and cleanedCurrentSettings is an object, so they differ, hence isDirty = true

knowing that we can of course work around it by checking for falsy values, but I still think that the component should not break when I'm using a legal combination of available properties - please let me know how you see it! (I'm still down to providing a one liner PR - EDIT: actually, it's already there, feel free to reject it if I did something wrong)

[Lukas742]

@superdyzio thanks for the explanation, I was now able to reproduce this bug.