camunda / camunda-modeler

An integrated modeling solution for BPMN, DMN and Forms based on bpmn.io.
https://camunda.com/products/modeler
MIT License
1.49k stars 479 forks source link

C7 properties panel breaks when template variable assignment is disabled #4382

Closed barmac closed 3 months ago

barmac commented 3 months ago

Describe the bug

C7 properties panel freezes when template variable assignment is disabled. The error thrown is related to the inputOutput missing unexpectedly, or outputParameter missing unexpectedly if there are more outputs on the element.

Steps to reproduce

  1. Apply the template below:
[
  {
    "name": "output-test",
    "id": "output-test",
    "appliesTo": [
      "bpmn:Task"
    ],
    "properties": [
      {
        "label": "output",
        "description": "disable assignment to reproduce",
        "binding": {
          "type": "camunda:outputParameter",
          "source": "name"
        }
      }
    ]
  }
]
  1. Switch toggle for variable assignment
  2. Properties panel freezes, and it does not update with element selection

Expected behavior

No crash.

Environment

Additional context

This is a regression.

SUPPORT-22412

barmac commented 3 months ago

This is first broken in https://github.com/bpmn-io/bpmn-js-element-templates/commit/86a3c0361dac623554125fc1d27afda0ccf501f3

barmac commented 3 months ago

I suspect https://github.com/bpmn-io/properties-panel/commit/bb1cfb05a41f3e7632ace7f34927e198e43ad775 as there were no big changes in the templates project in the marked commit apart from @bpmn-io/properties-panel version bump.

barmac commented 3 months ago

The direct cause of the problem is an unhandled error in the rendering when we try to get camunda:name of a non-existing output parameter. Somehow after the parameter is removed, we still try to render this component, although it should not be rendered according to this if: https://github.com/bpmn-io/bpmn-js-element-templates/blob/main/src/element-templates/properties-panel/properties/OutputProperties.js#L67 So this is a race condition. Correct flow is: toggle switch -> remove parameter -> render without AssignToProcessVariable; the actual flow is: toggle switch -> remove parameter -> rerender AssignToProcessVariable -> 💣

barmac commented 3 months ago

I've just confirmed that the bug appears when https://github.com/camunda/camunda-modeler/issues/4382#issuecomment-2178314298 is included.

barmac commented 3 months ago

My current hypothesis is that the incorrect rerender is due to naive key implementation which is based on the index of the object.

barmac commented 3 months ago

My current hypothesis is that the incorrect rerender is due to naive key implementation which is based on the index of the object.

Setting random keys doesn't help.

barmac commented 3 months ago

Today's findings from a session with @nikku :

ListGroup.js

  // we need to pass the proper (initial) open state when we 
  // initially render an element
  // 
  // at the same time, we compute the state _only_ in the second PASS
  // 
  // ==> So essentially, by design, we render _once_ with the old state.
  // ==> BOOM (!)
nikku commented 3 months ago

I have a bug fix in place that removes the aweful hacks we add in ListGroup:

image

To be integration tested (manually) tomorrow.

nikku commented 3 months ago

Turned out, as reported, to be a ListGroup related issue (old components used for re-rendering against new model).

Fixed upstream via https://github.com/bpmn-io/properties-panel/pull/369.

marstamm commented 3 months ago

Closed through https://github.com/camunda/camunda-modeler/pull/4392