Skip to content

Conversation

@barmac
Copy link
Member

@barmac barmac commented Sep 29, 2025

Proposed Changes

This changes the variables tracking so that we don't fail when expression cannot be parsed at all (#57). For a long broken expression, we entered ContextTracker#reduce and collected "variables" via first call. But for an immediately broken expression, we returned null which broke the mechanism. Perhaps we could have avoided this problem with proper type checking.

Try out via:

npx @bpmn-io/sr camunda/camunda-modeler -l bpmn-io/variable-resolver#additional-pr

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Sep 29, 2025
@barmac barmac requested review from Buckwich and nikku September 29, 2025 14:29
@nikku
Copy link
Member

nikku commented Sep 29, 2025

@barmac How does the problem manifest? I'd like to verify the fix E2E. I.e. would this fail previously?

image

Desktop Modeler v5.39.0

image

Preview environment

Spun up using npx @bpmn-io/sr bpmn-io/bpmn-js-properties-panel -l bpmn-io/variable-resolver#additional-pr.

image

➡️ I see that target is properly suggested now, despite the unparseable FEEL expression.

@barmac
Copy link
Member Author

barmac commented Sep 30, 2025

@barmac How does the problem manifest? I'd like to verify the fix E2E. I.e. would this fail previously?

In the current nightly, you get an unhandled error once you create an expression broken before first variable name, e.g. via ai agent connector:

Screen.Recording.2025-09-30.at.09.34.05.mov

@barmac
Copy link
Member Author

barmac commented Sep 30, 2025

I've added sr command in the top comment: #58 (comment)

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that the reported bug is fixed, and that test cases are present.

@barmac barmac merged commit 6f668ca into main Sep 30, 2025
5 checks passed
@barmac barmac deleted the additional-pr branch September 30, 2025 09:00
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants