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 bug in Explorer plugin where characters are dropped when typing quickly #3526

Merged
merged 6 commits into from Feb 1, 2024

Conversation

benjie
Copy link
Member

@benjie benjie commented Feb 1, 2024

To see a reproduction of this issue, check out: graphile/crystal#1845

The issue stems from some delays interfering with the reactivity model of React - when you type a character into Explorer, that change gets transformed into a GraphQL document string which is sent upstream into GraphiQL, GraphiQL then updates everything and sends the result back down stream to GraphiQL Explorer, which renders the new data. The issue is that if the user has typed another character in the interrim, their keypress will be overwritten by this flow because there is (I think!) a delay somewhere, i.e. the state is not updated synchronously. If the user types faster than this update loop, characters will be lost.

To solve this, we could use the "throttle" or "debounce" patterns to only send updates every so often or when the user is paused typing, but this leads to a less-than-instant experience for the user. Instead, I've opted to send the update upstream and then not send any further updates upstream until we receive back what we sent. If we get back what we sent then we send the next update (skipping all the intermediary keypresses and just jumping right to the current state), giving the user near-instant feedback on their typing. If what we receive is not what we sent then something else must have happened, and thus we overwrite our local state (which is the status quo). Through intense and rapid typing into both the explorer plugin and the operations text I was not able to trigger this branch, but nonetheless it is present to handle this situation.

The interface now feels buttery smooth to me, so I think this issue is solved.

To make the solution nice and clean I've put it in its own hook.

@benjie benjie requested a review from acao February 1, 2024 12:36
Copy link

changeset-bot bot commented Feb 1, 2024

🦋 Changeset detected

Latest commit: c887596

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

This PR includes changesets to release 4 packages
Name Type
@graphiql/plugin-explorer Patch
@graphiql/react Patch
@graphiql/plugin-code-exporter Patch
graphiql 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

Copy link
Contributor

github-actions bot commented Feb 1, 2024

The latest changes of this PR are available as canary in npm (based on the declared changesets):

graphiql@3.1.1-canary-e103fa61.0
@graphiql/plugin-code-exporter@1.0.4-canary-e103fa61.0
@graphiql/plugin-explorer@1.0.3-canary-e103fa61.0
@graphiql/react@0.20.3-canary-e103fa61.0

@acao
Copy link
Member

acao commented Feb 1, 2024

awesome work!! i wonder if we should add this logic to the hooks in graphiql/react actually though? perhaps with a denounce option and arguments? what do you think? i could see other plugins needing this

@benjie
Copy link
Member Author

benjie commented Feb 1, 2024

Sure; I've moved it into the react hooks as a hook result wrapper, so you can combine it e.g. const [state, setState] = useOptimisticState(useOperationsEditorState());

I considered making it an option (useOperationsEditorState({optimistic: true})) but since it requires a number of additional hooks (and branching is not allowed in hooks, so we couldn't opt out of them) I thought that this would be an unnecessary performance overhead for most usage of this hook - the wrapper approach seemed more efficient and clearer that there are performance side-effects (both good and bad).

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (25c3bfd) 55.56% compared to head (c887596) 55.34%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3526      +/-   ##
==========================================
- Coverage   55.56%   55.34%   -0.22%     
==========================================
  Files         115      115              
  Lines        5327     5348      +21     
  Branches     1444     1450       +6     
==========================================
  Hits         2960     2960              
- Misses       1941     1962      +21     
  Partials      426      426              
Files Coverage Δ
packages/graphiql-react/src/editor/hooks.ts 34.64% <0.00%> (-5.52%) ⬇️

@acao
Copy link
Member

acao commented Feb 1, 2024

awesome work!!!

Copy link
Member

@acao acao left a comment

Choose a reason for hiding this comment

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

looks great!

Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@acao acao merged commit 2b6ea31 into main Feb 1, 2024
13 checks passed
@acao acao deleted the explorer-performance-issue branch February 1, 2024 22:28
@acao acao mentioned this pull request Feb 1, 2024
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.

None yet

3 participants