Fix (graphql-language-service) Hover for first line#4162
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4162 +/- ##
==========================================
+ Coverage 63.96% 64.02% +0.05%
==========================================
Files 35 35
Lines 3086 3091 +5
Branches 949 949
==========================================
+ Hits 1974 1979 +5
Misses 1107 1107
Partials 5 5
🚀 New features to boost your workflow:
|
trevor-scheer
left a comment
There was a problem hiding this comment.
Thanks for the PR @lesleydreyer! At some point I'll have a closer look at this, but I left a couple comments in case you feel like poking around a bit more to get this right. Hopefully I'll have some more helpful comments for you in the near future.
| end: { character: 23, line: -1 }, | ||
| start: { character: 22, line: -1 }, |
There was a problem hiding this comment.
This smells funny, I suspect we need to normalize the off by 1 line number at the LSP/Monaco boundary where the line off by 1 is happening (presumably).
I can spend some more time investigating at some point but there's probably a more comprehensive solution that results in line numbers we'd actually expect here (presumably 0).
| // LSP may count from 1, but monaco counts from 0 | ||
| for ( | ||
| let i = 0; | ||
| location.line === 0 ? i <= location.line : i < location.line; |
There was a problem hiding this comment.
Related to the comment above, this doesn't feel like the correct place for this fix.
Hover is broken in `monaco-graphql`. On the first line it throws and renders nothing; on later lines it doesn't crash but highlights the wrong token. `GraphQLWorker.doHover` passes a 0-indexed position to `getRange`, which actually wants a 1-indexed `SourceLocation` (per the GraphQL spec, and what its other callers pass via `error.locations[0]`). On line 0 the loop never runs and `Expected Parser stream to be available` fires. On later lines `getRange` parses to end-of-line and ignores `column`, so the range comes back pointing at the previous line's last token. `getRange` was the wrong tool here anyway. It was built for diagnostics, not for finding the token under a cursor. `getTokenAtPosition` is what we want, and it's what the rest of the language service already uses. Fixes #4157 Fixes #3168 Supersedes #4162, which tried to special-case `line === 0` inside `getRange` itself. That left `getRange` returning a `Position` with `line: -1` (which becomes `0` after `toMonacoRange`'s `+1`, invalid in Monaco's 1-indexed API), and didn't address the bigger problem that `getRange` doesn't compute a token range to begin with. ## Validation ```bash yarn install yarn build yarn workspace example-monaco-graphql-react-vite dev ``` Open the URL Vite prints, with browser devtools open. 1. Hover the word `query` on the first line. Before: console logs `Error: Expected Parser stream to be available` and no popup appears. After: popup renders and `query` is highlighted. 2. Press Enter to push the query down a line, then hover a field name. Before: popup appears but the highlight is on the previous line's last token. After: the field under the cursor is highlighted.
fix hover for line 1 (0) in graphiql -> monaco-graphiql when hovering the first line
closes #4157 closes #3168