Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot add markers to BPMN view with swim lanes #660

Closed
jbiblio opened this issue Jan 24, 2024 · 8 comments
Closed

Cannot add markers to BPMN view with swim lanes #660

jbiblio opened this issue Jan 24, 2024 · 8 comments

Comments

@jbiblio
Copy link
Contributor

jbiblio commented Jan 24, 2024

When I deploy a BPMN with swim lanes, the addMarker JS function fails with an error.

  1. Create a simple BPMN with a swim lane. No sophisticated setup is needed, a simple hello-world with a swim lane is fine.
  2. Deploy the process and start a new instance
  3. Open the instance.

Actual outcome
The green borders are not shown:
image

An error is present in the Console:

Uncaught Error: invalid element specified
    at jt.add (bpmn-navigated-viewer.production.min.js:7:11426)
    at addElementInstanceCounter (app.js:610:14)
    at addMarkers (4503599627370510:437:4)
    at 4503599627370510:396:5
    at bpmn-navigated-viewer.production.min.js:29:1197
    at bpmn-navigated-viewer.production.min.js:29:2123
    at cr.open (bpmn-navigated-viewer.production.min.js:29:2130)
    at cr.importDefinitions (bpmn-navigated-viewer.production.min.js:29:1329)
    at bpmn-navigated-viewer.production.min.js:29:1092
    at bpmn-navigated-viewer.production.min.js:7:59827

Expected outcome
Now you should see the green markers around the task:
image

Investigation
In src/main/resources/templates/components/bpmn-diagram.html, the addMarkers function iterates over elementInstances. There is one instance present, that has no corresponding ID in the rendered diagram, AFAICS.

This causes an error in JS, so following elements from elementInstances are not marked properly.

Workaround
In src/main/resources/public/js/app.js, add try-catch in addElementInstanceActiveMarker and addElementInstanceIncidentMarker.
This catches the error from the "unknown" element.

See https://github.com/jbiblio/zeebe-simple-monitor/tree/fix-swimlanes.

But I'd consider this as a workaround.

I'm undecided about the correct solution:

  • The element in question should not be out into elementInstances in the first place, but I see no way to identify that element on Java side
  • The usage of bpmn.js is faulty, but I see no error in zeebe-simple-monitor's code
  • bpmn.js is faulty (an update may help, I did not tried this)
  • my workaround is correct, in which case I'm happy to PR it :-)

Any thoughts?

@nitram509
Copy link
Collaborator

Thanks for reporting the issue, Michael

I did realize, that the v6.5.1 of BPMN-JS is used.
This is very much outdated, and v16.4 is currently out.

After switching to the new version, I did run into another issue
bpmn-io/bpmn-js#2088

I hope to get an answer soon and would rather prefer upgrading the BPMN-JS and hopefully get this fixed in the lib.

If it's urgent, I would be fine to adopt your prepared patch ... just ping.

@jbiblio
Copy link
Contributor Author

jbiblio commented Jan 25, 2024

Hi Martin,

thanks a lot for checking out.

For me it's fine to wait, I applied the workaround locally.

@nitram509
Copy link
Collaborator

@jbiblio
I digged deep into the case and found, that when participants (swimlanes) are used, then the elementId is not the participants name attribute. I'm also not yet sure, what's the right thing to do and probably need to read the BPMN spec once more.

Long story short - I did upgrade to the latest BPMN-JS and implemented your try/catch proposal.
My local tests do look fine ... I you have the chance for a final test and report back,
I will bake a new release in a timely manner.

@jbiblio
Copy link
Contributor Author

jbiblio commented Jan 27, 2024

Hi @nitram509,

thanks a lot for digging deeper. I can check a pre-release or something like that next week.

Do you have a branch at hand I can integrate?

@nitram509
Copy link
Collaborator

Hi @nitram509,

thanks a lot for digging deeper. I can check a pre-release or something like that next week.

Do you have a branch at hand I can integrate?

Sure, just forgot to link it...
#665

@jbiblio
Copy link
Contributor Author

jbiblio commented Jan 31, 2024

Hi @nitram509 ,

my first tests look good, I asked the team to check as well. my team says it's fine as well :-)

@nitram509
Copy link
Collaborator

Hi @nitram509 ,

my first tests look good, I asked the team to check as well. my team says it's fine as well :-)

sounds great - thanks for the feedback.
I will bake a new release soon.

@nitram509
Copy link
Collaborator

Fixed in https://github.com/camunda-community-hub/zeebe-simple-monitor/releases/tag/2.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants