Groups are collapsed by their own first time on expand another group

[rumata28]

Describe the Bug

Steps to Reproduce

I had config of a section (this is my properties of "Message Intermediate Catch Event", where I am adding a custom ListGroup to standard "message" group):

Group "message"
|-- ListGroup "localCorrelationKeys" (my custom group)

I experienced bad UX:

• User expands the "message" group
• User expands the "localCorrelationKeys" nested ListGroup
• BAM - the system collapses all groups

This occurs just one time after diagram open. After that - it works normally.
It took hours to debug and I found the reason.

I removed all my customizations, but still found the bug in more simple scenario:

• Open diagram
• Select any element with Input and Output parameters

Group "input"
Group "output"

Those are Camunda properties - not mine, so, by removing my customizations, at least I eliminated that I did something wrong in my code by that.

• Expand "input"
• Expand "output"
• BAM - the system collapses all groups

And again - this happens just one time. After that it works normally.

Expected Behavior

Expected behavior is clearly, that system not do any unwanted collapsing of sections. Or at the very least do them somehow consistently.

Investigation results

All debugging efforts led me to the setLayoutForKey function: https://github.com/bpmn-io/properties-panel/blob/main/src/PropertiesPanel.js#L156

  const setLayoutForKey = (key, config) => {
    const newLayout = assign({}, layout);
    set(newLayout, key, config);
    setLayout(newLayout);
  };

At the very least, here we have the shallow copy, the min-dash assign function just redirect it to Object.assign:
https://github.com/bpmn-io/min-dash/blob/main/lib/object.js#L20
So, this is a shallow copy, while using set function from min-dash creates deep objects, like {group: {message: {open: true}}}

So the first thing - I tried replaced it to the following line:

    // commented out original line, because it creates a shallow copy
    // const newLayout = assign({}, layout);
    const newLayout = JSON.parse(JSON.stringify(currentLayout));

It changed behavior, but not in the way I wanted: it made behavior consistent - now every expand collapsed all other sections, every time, not just the first one.
I am not sure, guys, how do you prefer to create a deep copy of object, I did not find anything in min-dash, so came up with good old JSON.parse(JSON.stringify(...)).

Next thing I tried is instead of calling setLayout(newLayout), call it with a function like that:

  const setLayoutForKey = (key, config) => {
    setLayout(currentLayout => {
      const newLayout = JSON.parse(JSON.stringify(currentLayout));
      set(newLayout, key, config);
      return newLayout;
    });
  };

And this resolved the problem. Now, layout is updated as expected, first time and any subsequent expand of collapse action as expected.

I am still trying to understand, why this fixed issue. Probably the instance of currentLayout, which the setLayoutForKey possessed was obsolete for some reason, and using the callback is always updates the actual current value...

[rumata28]

For the time being, I found a workaround for my case:

import {set} from 'min-dash'
import {LayoutContext} from '@bpmn-io/properties-panel'
import {
  useContext,
  useCallback,
  useMemo
} from '@bpmn-io/properties-panel/preact/hooks'

/**
 * Layout State Bridge - fixes the buggy setLayoutForKey while preserving global state
 * This component reads from the global LayoutContext but provides a fixed setLayoutForKey function
 *
 * NOTE: this is a temporary workaround and should be removed once the underlying issue is fixed in the library.
 *       See https://github.com/bpmn-io/properties-panel/issues/443
 *
 * @example
 *   // Wrap ListGroup with LayoutStateBridge to fix layout state updates
 *   return (
 *     <LayoutStateBridge>
 *       <ListGroup
 *         element={element}
 *         id={id}
 *         label={translate(label)}
 *         items={listGroupItems}
 *         add={addMapping}
 *       />
 *     </LayoutStateBridge>
 *   )
 */
export function LayoutStateBridge({children}) {
  const originalLayoutContext = useContext(LayoutContext)

  // Fixed version of setLayoutForKey that uses functional state update
  const setLayoutForKey = useCallback((key, config) => {
    // Use the original setLayout but with functional update to prevent stale closures
    originalLayoutContext.setLayout(currentLayout => {
      // Deep copy to prevent mutations
      const newLayout = JSON.parse(JSON.stringify(currentLayout))
      set(newLayout, key, config)
      return newLayout
    })
    // eslint-disable-next-line
  }, [originalLayoutContext.setLayout])

  const fixedContextValue = useMemo(() => ({
    ...originalLayoutContext,
    setLayoutForKey
  }), [originalLayoutContext, setLayoutForKey])

  // noinspection JSValidateTypes
  return (
    <LayoutContext.Provider value={fixedContextValue}>
      {children}
    </LayoutContext.Provider>
  )
}

For anyone who need it - all you need is just to wrap your instances of ListGroup / Group in this LayoutStateBridge and it get to work without the bug.

[barmac]

Do you have any idea why this problem cannot be reproduced in bpmn-js-properties-panel or Desktop Modeler? Can you share a codesandbox where one can reproduce it?

[rumata28]

Hello!

I ask for an apology, but I have no idea why it does work in Desktop Modeler - I have never used it.

But I managed to create a unit-test exactly for the scenario:

• rumata28@87b2179

The scenario is as simple as expanding "Group 1" and then expanding "Group 2".
That's it.

Due to flaws in the setLayoutForKey logic it does not work, creating a really frustrating effects for user.

Specifically, this is exactly what shall not fail (and it does fail):

      // then - and also the first one shall STILL be expanded
      // TODO: BAM! here is where it fails
      expect(isGroupOpen('group-1')).to.be.true;

Group 1 collapses, not maintaining its open state when Group 2 open.

[barmac]

Thanks, I will look into this.

[barmac]

I can reproduce the problem. Probably the reason why we cannot reproduce it in the downstream libraries is because we persist the layout separately. Still, this needs more investigation. Thanks for providing the test case. That shows the issue clearly.