appsmithorg / appsmith

Platform to build admin panels, internal tools, and dashboards. Integrates with 25+ databases and any API.
https://www.appsmith.com
Apache License 2.0
33.96k stars 3.66k forks source link

[Bug]-[1000]:Incorrect bracket groups are paired #6923

Open ramsaptami opened 3 years ago

ramsaptami commented 3 years ago

Description

Brackets are incorrectly grouped together image image

Important Details

Please Read below comment for resolution history

https://github.com/appsmithorg/appsmith/issues/6923#issuecomment-1235616471

rishabhrathod01 commented 2 years ago

Hey team! Please add your planning poker estimate with ZenHub @ajinkyakulkarni @ApekshaBhosale @hetunandu @ohansFavour

ApekshaBhosale commented 2 years ago

@Rishabh-Rathod is already looking into it. looks like a codeMirror issue

rishabhrathod01 commented 2 years ago

@ramsaptami the red color (only for text and not bg) braces here mean these braces have no matching pairs, according to how codemirror handles it.

Is this confusing and any suggestion on how can we improve it? cc: @ajinkyakulkarni @rohan-arthur @ramsaptami

Also, there is an issue where it doesn't remove those even after the typing cursor is not pointing it.

bharath31 commented 2 years ago

Assuming 10% of users who write a long-expression on the property pane face this issue

Stats

Stat Values
Reach 500
Effort (months) 0.5
rishabhrathod01 commented 2 years ago

History of solving the issue

Root cause

This issue was regarding codemirror's bracket matching logic which lies in node_modules/codemirror/addon/edit/matchbrackets.js where if there was an invalid bracket added in code then the logic couldn't ignore it.

For example:

function a (){
  ) // this is an invalid bracket causing a mismatch of braces
}

To fix this issue, we tried to go through the codeMirror logic and change the logic to resolve the issue.

code - https://github.com/appsmithorg/appsmith/compare/release...fix-match-mustaches

As it was a high-effort task with low impact. It was deprioritized to be picked up later.

Future suggestions to solve this issue

Codemirror 6 handles such bracket pairing correctly which means no effort will be needed to resolve this issue after migration.

Demo of codemirror 6: https://codesandbox.io/s/f6nb0

As codemirror 6 is now stable and has benefits like supporting multiple languages together ( multiplexing ) migrating might make more sense to us when compared to putting effort to fix old addon logic.

satbir121 commented 10 months ago

Adding another screenshot where the pairing is wrong

Screenshot 2023-11-21 at 10 18 25 AM