Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

fix(graphql-language-service): fix off by one on hover #3882

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

Open
bensengupta wants to merge 5 commits into graphql:main
base: main
Choose a base branch
Loading
from bensengupta:main

Conversation

@bensengupta
Copy link

@bensengupta bensengupta commented Mar 16, 2025 •
edited
Loading

Hey, thanks for making this! This PR fixes a small off-by-one issue when hovering.

Details I am using graphql-language-service-cli in Neovim. The issue is illustrated below:

Hover, no information (incorrect, should show info)
Screenshot 2025年03月16日 at 6 37 17 PM
Hover, shows information (correct)
Screenshot 2025年03月16日 at 6 37 22 PM
Hover, shows information (incorrect, should not show info)
Screenshot 2025年03月16日 at 6 37 27 PM
Hover, no information (correct)
Screenshot 2025年03月16日 at 6 37 31 PM

Copy link

changeset-bot bot commented Mar 16, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 14cca83

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

This PR includes changesets to release 2 packages
Name Type
graphql-language-service Patch
codemirror-graphql 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

linux-foundation-easycla bot commented Mar 16, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@bensengupta bensengupta marked this pull request as ready for review March 16, 2025 22:49
Copy link
Author

Seems like there is a race condition in the tests. Am unable to reproduce test failure locally.

Maybe replace this

if (!existsSync(this._tmpDirBase)) {
void mkdirSync(this._tmpDirBase);
}

with this?

 try {
 void mkdirSync(this._tmpDirBase);
 } catch (err) {
 if (err instanceof Error && 'code' in err && err.code === 'EEXIST') {
 return;
 }
 throw err;
 }
schema: GraphQLSchema,
contextToken?: ContextToken,
options?: { mode?: GraphQLDocumentMode; uri?: string },
offset = 0,
Copy link
Author

@bensengupta bensengupta Mar 18, 2025

Choose a reason for hiding this comment

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

Just to explain this change: getContextAtPosition is used in 2 places:

In getHoverInformation, the offset needs to be changed to 0 to fix the off-by-one problem on hover.

const context = getContextAtPosition(queryText, cursor, schema, contextToken);

In getAutocompleteSuggestions, the offset needs to be kept at 1 as the current behavior is correct.

const context = getContextAtPosition(
queryText,
cursor,
schema,
contextToken,
options,
1,
);

Copy link
Author

Hey @acao @dimaMachina, any chance one of you could have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@johnaAr555 johnaAr555 johnaAr555 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /