Fix CypherEditor state handling and add tests

[danieladugyan]

This PR once again attempts to fix the issues with how the CypherEditor handles state updates. I'll try to explain below what the problem is and how we could solve it.

TL;DR the CypherEditor now flushes debounced changes before onExecute, and handles this.prop.value updates by checking if this.props.value is different from prevProps.value.

Problems

Problem 1: value not cleared onExecute

1. Set up a controlled CypherEditor component as follows:

   const [value, setValue] = useState('');
   return <CypherEditor value={value} onChange={setValue} onExecute={() => setValue('')} />

2. Input "foo" and then immediately press Enter. This must be done quickly; the component debounces its onChange calls with a 200 ms timer. This means onChange is called 200 ms after user stops typing instead of being called on every keystroke.
3. Pressing Enter calls setValue('') which of course causes the component to re-render with an empty string as value. However, there's still an onChange call waiting to happen, and when it fires the value will be set to "foo" again.

Solution 1

There's a relatively straightforward solution to this problem. After setValue('') is called, the component's componentDidUpdate() method will be called. Here, we can simply check if the value has changed, and it if it has, cancel the debounced onChange call. This ensure we can't accidentally override a setValue('') call with pending inputs.

// When the component re-renders, check if the value prop is different from the editor text.
// This means setValue() has been called with something other than the editor text. Right?
componentDidUpdate(prevProps): {
  if (this.props.value !== editorText) {
    this.debouncedOnChange?.cancel();
    editorView.dispatch(this.props.value); // sync editorText with this.props.value (abridged)
  }
}

Problem 2: programmatic input not working

1. Set up a controlled CypherEditor component again.
2. Input "foo" and then immediately re-render the component. Why? Well, this happens in practice sometimes. For instance, another component might programmatically input a value (i.e. using setValueAndFocus("foo")) and subsequently cause a re-render.
3. The component will re-render and its componentDidUpdate() method will be called. We'll once again reach the code below:

   // this.props.value === "", editorText === "foo"
   componentDidUpdate(prevProps): {
     if (this.props.value !== editorText) {
       this.debouncedOnChange?.cancel();
       editorView.dispatch(this.props.value);
     }
     
   }

   Notice that the problem is similar to the situation we had previously, but it is reversed. value is lagging behind the editorText and the component is re-rendering following an edit to the editorText, as opposed to through setValue(). In this situation we must not cancel the debounced onChange. If we do, the input will just stay empty.

Solution 2

Instead of checking whether this.props.value is different from the editorText, we can check whether it has changed from the previous props. If the component is just re-rendering, the value will be unchanged and we'll know not to cancel the pending onChange call.

componentDidUpdate(prevProps): {
  if (this.props.value !== prevProps.value) {
    this.debouncedOnChange?.cancel();
    editorView.dispatch(this.props.value);
  }
}

Problem 3: value still not cleared onExecute

However, the component is still not working as expected. The first problem is actually a bit of an edge case. The component's value was empty initially, then we typed some input and immediately pressed Enter. If we're quick enough, both this.props.value and prevProps.value will be ''! According to our logic, this means that setValue('') has not been called, but rather the component is just re-rendering. Therefore, we must conclude that there is no way to distinguish a setValue() call from a re-render, at least in some cases.

Conclusion

Option 1

As far as I can tell there is no way to address the underlying problem with having a debounced onChange function. When componentDidUpdate is called, we don't know whether to cancel upcoming onChange calls or not. However, in practice, problematic setValue calls only seem originate from the onExecute callback. For most practical applications, we can therefore solve the problem by flushing upcoming onChange calls before calling onExecute. This ensures that the setValue calls happen in their expected order. However, note that the component will continue to override setValue calls originating from outside onExecute (e.g if while the user is typing setValue('foo') is fired for some reason, then the value will be reset to whatever the user was typing). See the tests that have been annotated with test.fails for concrete examples.

Option 2

The ideal solution would be to remove the debounce entirely. Instead, we can just call onChange immediately. However, I expect this will lead to performance problems. It would therefore be interesting to attempt to move the debounce logic to the specific method calls causing the performance problems (e.g. autocomplete, syntax highlighting etc?). Instead of debouncing the onChange call, we would instead be debouncing the slow methods themselves.

Flowchart

Further reading

Debouncing and Throttling Explained Through Examples
https://codemirror.net/docs/ref/#autocomplete.autocompletion%5Econfig.activateOnTypingDelay

[changeset-bot]

🦋 Changeset detected

Latest commit: c211e25

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@neo4j-cypher/react-codemirror	Patch
@neo4j-cypher/react-codemirror-playground	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[danieladugyan]

Fix CypherEditor's state synchronization logic

[danieladugyan]

Unit tests are sometimes failing with [vitest-worker]: Timeout calling "fetch" with.... It's not clear why, but others seem to have the same issue.

vitest-dev/vitest#6131
https://github.com/vitest-dev/vitest/blob/99452a712c83e4e90a8afd5675e6573e1c22a43a/packages/vitest/src/runtime/rpc.ts#L83

UPDATE: Fixed by restricting the number of workers to 1. I think this is an acceptable workaround given the small number of unit tests in the react-codemirror package.

[OskarDamkjaer]

Of the two options I think the first one makes most sense.

If you want to investigate the second option here's some context:

I added the debounce here, along with a few other changes to prevent what was before heavy input lag. In your linked codemirror doc we can see that autocompletion calls are already debounced-ish with 100ms, and as I remember it when I profiled it the input lag it was the frequent react re-renders that were problematic. My suggestion would be to profile / compare editing the largePokemonquery (and some mid sized query) in benchmarkQueries in query with and without the debounce to confirm that the problem is with react re-rendering the component or something in codemirror that's slowing it down.

A third option that I think you said @daveajrussell originally suggested could also be interesting: don't support changing the value directly when using a controlled component. Consumers will have to chose between an uncontrolled component where they need to read and set the value manually via the ref (possibly through the setValueAndFocus) or use a controlled component and set it through the value prop. I've not thought it through 100% but I think that'd solve the issue in a clean/sustainable way right?

[danieladugyan]

Of the two options I think the first one makes most sense.

Agree.

If you want to investigate the second option here's some context:

I added the debounce #158, along with a few other changes to prevent what was before heavy input lag. In your linked codemirror doc we can see that autocompletion calls are already debounced-ish with 100ms, and as I remember it when I profiled it the input lag it was the frequent react re-renders that were problematic. My suggestion would be to profile / compare editing the largePokemonquery (and some mid sized query) in benchmarkQueries in query with and without the debounce to confirm that the problem is with react re-rendering the component or something in codemirror that's slowing it down.

I'm not sure I'm ready to go down another rabbit hole of performance profiling and optimization. 😄

A third option that I think you said @daveajrussell originally suggested could also be interesting: don't support changing the value directly when using a controlled component. Consumers will have to chose between an uncontrolled component where they need to read and set the value manually via the ref (possibly through the setValueAndFocus) or use a controlled component and set it through the value prop. I've not thought it through 100% but I think that'd solve the issue in a clean/sustainable way right?

Instinctively I don't think this solves problem 3. In theory, even if we disallow changing the value through the ref, that's still functionally identical to simply typing quickly and thus the same problems should exist.

[OskarDamkjaer]

@danieladugyan was the problem with the lastDispatchedValue fix not that it didn't properly handle the external setting from setValueAndFocus? I might be misremembering

[danieladugyan]

@danieladugyan was the problem with the lastDispatchedValue fix not that it didn't properly handle the external setting from setValueAndFocus? I might be misremembering

I'm honestly not sure. However, I discounted lastDispatchedValue as a viable solution for the same reasons as my other solutions. Again, it doesn't fundamentally address problem 3: how do you know when lastDispatchedValue should be updated inside componentDidUpdate? It needs to be updated, otherwise it will go out of sync. But it wouldn't be correct to update it during a re-render.

Furthermore, even if lastDispatchedValue is correctly implemented then it should always be the same as prevProps.value, right?

[OskarDamkjaer]

Furthermore, even if lastDispatchedValue is correctly implemented then it should always be the same as prevProps.value, right?

Not necessarily, you could change the value prop externally two times for example. But yes, you're right, it still doesn't solve the problem.

[OskarDamkjaer]

Looking good from my POV

[daveajrussell]

I've installed this in my local upx/query repo and I'm observing a few issues;

• the suggestions are "sometimes" being dismissed (I think) on the debounce
  

• entering certain syntax (like ellipses) forces the caret back to the start of the input
  

should this change be able to plug straight in without any modifications to consumers?

I was having a little play with the query implementation.. removing the onChange handler that sets the codemirror value programmatically fixes the above 2 issues, but introduces an issue where the main editor is not cleared when a cmd is executed 😅

[daveajrussell]

Tested out in my upx repo, looks good 👍