close
Skip to content

Fix error encountered running in helix#4123

Open
forivall wants to merge 4 commits into
graphql:mainfrom
forivall:patch-1
Open

Fix error encountered running in helix#4123
forivall wants to merge 4 commits into
graphql:mainfrom
forivall:patch-1

Conversation

@forivall
Copy link
Copy Markdown

I manually applied this fix and the lsp works now. Single character fix to add a missing ?.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Oct 29, 2025

🦋 Changeset detected

Latest commit: 2230bf8

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

This PR includes changesets to release 1 package
Name Type
graphql-language-service-server 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
Copy Markdown
Contributor

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

@forivall change seems reasonable to me, but can you share a reproduction that demonstrates the issue?

@forivall
Copy link
Copy Markdown
Author

forivall commented Apr 17, 2026

Here's my reproduction repository: https://github.com/forivall/gql-config-repro-workspace/tree/89204bd11c78655db950237db8264e205f624d90

setup: npm install -g graphql-language-service-cli and helix editor. helix has graphql-lsp support out of the box.

to reproduce, i first watch the helix logs via tail -f ~/.cache/helix/*.log | rg --max-columns 10000 graphql-language-service

and then run hx -vvv test-files/1.ts

Among the log lines, i will see

2026-04-17T09:52:55.949 helix_lsp::transport [INFO] graphql-language-service <- {"jsonrpc":"2.0","method":"window/logMessage","params":{"type":1,"message":"TypeError: Cannot read properties of undefined (reading 'length')"}}

and the error will not appear.

The expected behaviour, which is how it works when i apply my fix, is to show this error:

ScreenShot 2026-04-17 at 9 55 18 AM

(the files for the above repo were adapted from the tests for graphql-codegen)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you improve type for settings?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

this._settings = { ...settings, ...vscodeSettings };
const rootDir = this._settings?.load?.rootDir.length
const rootDir = this._settings?.load?.rootDir?.length
? this._settings?.load?.rootDir
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
? this._settings?.load?.rootDir
? this._settings.load.rootDir

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.96%. Comparing base (8f25b38) to head (2230bf8).
⚠️ Report is 42 commits behind head on main.

Files with missing lines Patch % Lines
...ql-language-service-server/src/MessageProcessor.ts 33.33% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4123   +/-   ##
=======================================
  Coverage   63.96%   63.96%           
=======================================
  Files          35       35           
  Lines        3086     3086           
  Branches      949      934   -15     
=======================================
  Hits         1974     1974           
  Misses       1107     1107           
  Partials        5        5           
Files with missing lines Coverage Δ
...ql-language-service-server/src/MessageProcessor.ts 89.22% <33.33%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

3 participants