Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Fix cursor jumping when typing in GraphQL API console#57862

Merged
pjlast merged 6 commits into
mainfrom
pjlast/graphql-console-cursor-jump-fix
Nov 8, 2023
Merged

Fix cursor jumping when typing in GraphQL API console#57862
pjlast merged 6 commits into
mainfrom
pjlast/graphql-console-cursor-jump-fix

Conversation

@pjlast

@pjlast pjlast commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

Naive attempt at fixing this very annoying bug in the GraphQL API console where the cursor jumps back while you're typing: https://www.loom.com/share/e5d50b5f6bcd4a8eb6ae08cc6a3832f2

This happens because this.setState is called on every keypress when the query gets updated, which causes a component rerender. It seems like when the component and the state go out of sync, it rerenders completely, causing the cursor to jump back.

This PR makes a small change so that this.setState is not called in the onEditQuery function, and only the URL gets updated. This means that a link to the query is still shareable, but it shouldn't trigger the mass re-rendering it's currently doing.

Test plan

Tested by typing a whole bunch. Also made sure the URLs are still shareable.

@cla-bot cla-bot Bot added the cla-signed label Oct 24, 2023
@github-actions github-actions Bot added the team/source Tickets under the purview of Source - the one Source to graph it all label Oct 24, 2023
@pjlast pjlast requested a review from a team October 24, 2023 14:11
@sourcegraph-bot

sourcegraph-bot commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

Comment thread client/web/src/api/ApiConsole.tsx Outdated
Comment on lines +184 to +185
private onEditQuery = (newQuery?: string): void =>
this.updateStateParameters(parameters => ({ ...parameters, query: newQuery }))
this.updates.next({...this.state.parameters, query: newQuery})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

reading through this component, this means that the state in this.state.parameters will get outdated over time, right?
We pass that to GraphiQL as the query field, so I wonder if other reasons to rerender might cause it to reset now?
Or is the query field basically the first value to use and never read again?

If this is not the case, I think we should drop the update state parameters thing entirely and also just call setState for variables and operationName as well for consistency, wdyt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So I tried breaking this by trying to get it to rerender in other ways, and it just doesn't 🤷‍♂️

From the source code comments, it seems like the query param is there if you want to control the query of the editor programmatically. But in the way we're using it, we're basically doing both. The user is typing, and then we're setting it again to what the user typed, which causes the weird race condition.

But yeah could definitely see if we can remove the query state completely then

@pjlast pjlast force-pushed the pjlast/graphql-console-cursor-jump-fix branch from 073bcac to 09d9279 Compare November 6, 2023 13:51
@pjlast pjlast enabled auto-merge (squash) November 8, 2023 14:09
@pjlast pjlast merged commit 655660a into main Nov 8, 2023
@pjlast pjlast deleted the pjlast/graphql-console-cursor-jump-fix branch November 8, 2023 14:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants