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

FEEL error reported switches #3845

Closed
nikku opened this issue Sep 11, 2023 · 6 comments · Fixed by #3874
Closed

FEEL error reported switches #3845

nikku opened this issue Sep 11, 2023 · 6 comments · Fixed by #3874
Assignees
Labels
Milestone

Comments

@nikku
Copy link
Member

nikku commented Sep 11, 2023

Describe the bug

Using the modelers validation feature leads to different errors flashing when invalid FEEL content is entered to a field:

capture CEBm8o_optimized

It looks like two different validations kick in, resulting in different error messages.

Steps to reproduce

  1. Model a service task
  2. Enter broken FEEL expression in type field
  3. Re-focus node
  4. See error message switching (cf. above).

Expected behavior

A clear, meaningful, and stable error message is shown to me as a user. At best it indicates that FEEL is the format of the (expected) language. If possible the editor itself establishes additional context (red caret?).

Environment

  • OS: Linux
  • Camunda Modeler Version: v5.15.0
  • Execution Platform: Any
  • Installed plug-ins: None

Additional context

No response

@nikku nikku added bug Something isn't working feel editing properties panel labels Sep 11, 2023
@nikku nikku changed the title FEEL error reported FEEL error reported switches Sep 11, 2023
@philippfromme
Copy link
Contributor

@philippfromme philippfromme added the ready Ready to be worked on label Sep 11, 2023
@smbea
Copy link
Contributor

smbea commented Sep 14, 2023

This happens because the syntax error takes a while to come through, so it uses the error it already has.
I'm not sure it's possible to speed up how the error analysis in the feel editor (I'm not familiar with it but I didn't see any bug that needed fixing). So what should be our strategy for these cases? Maybe delaying would work, but it seems a bit like patch work.

@philippfromme
Copy link
Contributor

I think the hierarchy of const error = localError || temporaryError || validationError; is quite arbitrary. First local, then global, then local again? We could decide to just change it to const error = temporaryError || localError || validationError;, meaning const error = globalError || localError || editorError;.

@smbea
Copy link
Contributor

smbea commented Sep 15, 2023

@philippfromme So in these cases, we prioritize the linting over the syntax error. Meaning the user just gets a "not valid" feedback

@barmac
Copy link
Collaborator

barmac commented Sep 15, 2023

If I understand this correctly, the human-readable "This is not a valid FEEL expression" would get priority, but user could still see the exact editor error if they highlight the error.
image

Makes sense to me.

@smbea
Copy link
Contributor

smbea commented Sep 15, 2023

Ah that makes sense then, didn't think of that. Thanks for explaining. I vouch for this then and will follow up.

smbea added a commit to bpmn-io/properties-panel that referenced this issue Sep 15, 2023
smbea added a commit to bpmn-io/properties-panel that referenced this issue Sep 15, 2023
smbea added a commit to bpmn-io/properties-panel that referenced this issue Sep 18, 2023
@smbea smbea added fixed upstream Requires integration of upstream change and removed ready Ready to be worked on labels Sep 18, 2023
nikku added a commit that referenced this issue Sep 21, 2023
Should fix FEEL popup editor.
Should fix #3845
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed fixed upstream Requires integration of upstream change labels Sep 22, 2023
nikku added a commit that referenced this issue Sep 22, 2023
Should fix FEEL popup editor.
Should fix #3845
nikku added a commit that referenced this issue Sep 22, 2023
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Sep 22, 2023
nikku added a commit that referenced this issue Sep 22, 2023
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 22, 2023
@nikku nikku added this to the M69 milestone Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants