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

Fix for random UI crash #2840

Merged
merged 7 commits into from
Nov 24, 2020
Merged

Fix for random UI crash #2840

merged 7 commits into from
Nov 24, 2020

Conversation

MrSnix
Copy link
Contributor

@MrSnix MrSnix commented Nov 16, 2020

After some analysis i found out the main problem it's related to the from.line which has an undefined value in some situations (which i was not able to reproduce). foldOptions.widget(from, to) is called internally by CodeMirror when you want to fold code inside the editor itself. This makes React crash when the "Send" button is pressed because the CodeMirror state is not consistent due to invalid folding triggering an infinite setState.

I was thinking about simply return and do not execute any folding on the editor but it will create another inconsistency due to CodeMirror attaching an eventListener on an invalid point. So maybe throwing an error could be the best option.
At the least CodeMirror won't make the app crash when it's not able to retrieve from which line the folding happens and won't attach eventListener on invalid point.

This my proposal for a fix, it closes #2805

Any feedback?

@MrSnix MrSnix changed the title Fix for random UI crash #2805 Fix for random UI crash Nov 16, 2020
@MrSnix MrSnix mentioned this pull request Nov 16, 2020
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Thanks for investigating a submitting a PR! 🎉

I think that instead of throwing an error, we simply return '\u2194' (the current fallback, see line 330) if either from or to are falsey, and allow CodeMirror to handle it internally. The event listener would likely be created regardless, because this code doesn't actually do any folding, it only provides the text content. There will be an arrow in the gutter (see below) to fold/unfold regardless.

image

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Nice 😄 a few more changes please!

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

🎉 LGTM, thanks for the PR

Copy link
Contributor

@DMarby DMarby left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for investigating and fixing this!

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.

UI crash
3 participants