feat: adjust and apply multi-instance execution listener 'Before All' type errors#155
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for the new multi-instance execution listener beforeAll compatibility validations and updates UI-facing lint messages for the problem panel and properties panel.
Changes:
- Enables/bundles new
before-all-execution-listenerandno-before-all-execution-listenerrules. - Adds adjusted error messages and properties-panel entry mapping for
beforeAll. - Updates related dependencies and adds regression tests.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Documents the feature and dependency update. |
lib/compiled-config.js |
Bundles the new compatibility rules. |
lib/utils/error-messages.js |
Adjusts problem-panel messages for beforeAll execution listener errors. |
lib/utils/properties-panel.js |
Maps unsupported beforeAll event type errors to the execution listener event type field. |
package.json |
Updates compat and properties-panel related dependencies. |
package-lock.json |
Locks updated dependency versions. |
test/spec/utils/error-messages.spec.js |
Adds tests for adjusted problem-panel messages. |
test/spec/utils/properties-panel.spec.js |
Adds a test for properties-panel mapping of unsupported beforeAll. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if ( | ||
| isPropertyError(data, 'eventType', 'zeebe:ExecutionListener') | ||
| && data.allowedVersion |
There was a problem hiding this comment.
This is intentional. Version-gated "beforeAll" rule gets the full inline treatment (highlight + message), MI-only rule stays Problems-panel only — such cases are auto-cleaned by CleanUpExecutionListenersBehavior, so there is no case when we would see 'Before All' event type on non MI element in properties-panel.
There was a problem hiding this comment.
Shouldn't we just have a general eventType properties panel integration? This way we wouldn't have to modify the implementation when new even types are added, e.g. upcoming cancel event.
There was a problem hiding this comment.
Also I think the copilot's remark is valid that the allowed version is irrelevant for the properties panel integration.
There was a problem hiding this comment.
Shouldn't we just have a general eventType properties panel integration? This way we wouldn't have to modify the implementation when new even types are added, e.g. upcoming cancel event.
Agree that would be more generic, I'll fix it.
Also I think the copilot's remark is valid that the allowed version is irrelevant for the properties panel integration.
Do you mean that we don't want to show such an 'Unsupported version' error in the properties panel at all? Right now, we already have quite a lot of the same cases(e.g. 1, 2)
There was a problem hiding this comment.
Shouldn't we just have a general eventType properties panel integration? This way we wouldn't have to modify the implementation when new even types are added, e.g. upcoming cancel event.
There was a problem hiding this comment.
Do you mean that we don't want to show such an 'Unsupported version' error in the properties panel at all?
No, in the context of the highlighted change we require the allowed version in the condition, so the entry ID is not provided unless the error contains the allowed version. Logically the allowed version is irrelevant when we wonder if the properties panel entry should be focussed or not.
There was a problem hiding this comment.
Got it, makes sense. I've already fixed it this way.
If it looks good now and that was the last thing to fix, can i merge it?
| if ( | ||
| is(node, 'zeebe:ExecutionListener') | ||
| && property === 'eventType' | ||
| && node.get('eventType') === 'beforeAll' | ||
| ) { | ||
| return 'An <Execution listener> with <Before all> event type is only allowed on multi-instance elements'; | ||
| } |
There was a problem hiding this comment.
This is a no-op, isn't it? We never allow to create such listeners in the first place, do we?
There was a problem hiding this comment.
Yes, this is a quite hypothetical case, since we will clear the 'Before all' event type if MI is removed from the element. The only way this could happen is through an external change to the XML or if our behaviors are disabled. However, I decided to add this error just as an additional guard from the linting side
There was a problem hiding this comment.
Are there any other cases where we support situations impossible in the Modeler?
There was a problem hiding this comment.
I'm not aware of them. My intention was that adding a linting rule is a cheap, additional guard for unexpected cases or if behaviors don't do the cleanup (in case of future bugs). But if this is not the approach we use, I can remove it. Also, in this case such a rule probably should be removed from bpmnlint-plugin-camunda-compat, or do we assume that this can be used by other vendors and it's better to keep it for cases when they don't use our behaviors?
There was a problem hiding this comment.
bpmnlint plugin is used outside of the Modeler context too, but camunda/linting is a Modeler-specific solution. On the other hand, with the rise of AI-generated diagrams, we may as well want to display human-readable messages in the UI to enable humans to fix the problem. This conversation is not a blocker for the PR to me.
… type errors Related to camunda/camunda-modeler#5869
…iors` to 1.15.0 versions.
AlekseyManetov
force-pushed
the
multi-instance-execution-listeners-linting
branch
from
6508056 to
91e4d46
Compare
Related to camunda/camunda-modeler#5869
Proposed Changes
Adjust multi-instance execution listener 'Before All' type errors for problem panel and add 'unsupported version' error into properties panel.
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/srtoolCloses {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}