Skip to content

codemirror: Fix token hover errors (caused by extensions derived from props)

Administrator requested to merge fkling/cm-extensions into main

Created by: fkling

While working on other CodeMirror query input related stuff, I came across this obscure issue: CodeMirror would throw a bunch of errors related to our own token highlighting extension.

RangeError: Field is not present in this state

2022-06-22_14-56_1

This would appear when hovering over query tokens after typing into the search query input. It didn't make much sense to me because the highlightedTokenPosition field is clearly always present. Luckily this doesn't seem to break the editor as a whole.

Eventually I looked into how often the whole tokenInfo extension was created and it turns out that it was created multiple times, more precisely the extension would be re-created on every key press. Even worse, it looks like the old DOM event handlers are not detached when the tokenInfo extension is replaced with a new version (this can be seen by the fact that the hover event triggers event handlers from two different extension instantiations):

https://user-images.githubusercontent.com/179026/175117671-ebb1f9ad-eba8-420b-84c1-96c1e999d1f1.mp4

This meant two things:

  1. Some props that the extensions depend on change on every keypress.
  2. There is either a bug in CodeMirror or how we create extenions that cause old DOM event handlers from staying around.

I first looked into which props changed and the culprit seems to be onCompletionItemSelected, which causes extensions to be recreated:

2022-06-22_14-56

There are a couple of ways to go about this. Ideally onCompletionItemSelected would be stable, and I implemented that in the second commit (bb2916b60484c47f50f6b04fd938a2a364f44274). This also includes a change to avoid parsing the query in the first place when the tour is not enabled.

But that fix alone would mean that people would still have to know to pass stable values to the query input to avoid this error. So ideally there is a way for the query input itself to deal with this problem.

I actually already implemented the onSubmit handler in such a way that it avoids recreating all extensions if just that handler changes (because that is a common) (tl;dr: The handler is stored in a state field and the field is updated via a transaction). But doing this for every callback function seemed cumbersome, and was still an abstraction level too high for my taste (and would have resulted in a lot of additional code).

Interestingly, CodeMirror has the concept of "compartments" which allows you to replace/update a subset of extensions. The "bad extension" came from CodeMirrorMonacoFacade but tokenInfo is provided by the lower level component CodeMirrorQueryInput (whether or not the current separation of extensions in two different components make sense is another topic). So a simple fix was to compartmentalize all extensions passed from CodeMirrorMonacoFacade to CodeMirrorQueryInput, so that only the "higher level" extensions would be recreated but not CodeMirrorQueryInput's own extensions (side note: I think that's a great solution for building layers of CodeMirror components). This was implemented in the first commit.

Test plan

  • Open search page and dev tools.
  • Type in query and hover over query tokens.
  • No errors should be shown in the console.

Integration tests pass.

App preview:

Check out the client app preview documentation to learn more.

Merge request reports