Respect server textDocumentSync capability for didChange notifications#194
Respect server textDocumentSync capability for didChange notifications#194
Conversation
Solargraph (and other servers) advertise textDocumentSync: 1 (Full), meaning they expect the complete document text on every didChange notification rather than incremental ranges. Sending incremental changes to a Full-sync server leaves it with a stale view of the document, preventing type inference for local variables. Now lsp_setup_buffer_hooks reads textDocumentSync from server capabilities and sends either a full document or an incremental range accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for respecting the server's textDocumentSync capability in LSP integrations. Previously, the code always sent incremental document changes, which caused issues with servers like Solargraph that expect full document synchronization. The change enables proper type inference for local variables by sending the complete document text when required.
Changes:
- Modified
lsp_setup_buffer_hooksto read and respect the server'stextDocumentSynccapability - Added conditional logic to send either full document text (sync kind 1) or incremental changes (sync kind 2) based on server capabilities
- Updated comments to document the two sync modes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| when Hash then sync["change"].to_i | ||
| else 2 | ||
| end | ||
|
|
There was a problem hiding this comment.
The new textDocumentSync capability handling logic lacks test coverage. Consider adding tests to verify the behavior for different sync kinds (Full, Incremental, None) and different textDocumentSync formats (Integer vs Hash with "change" field).
| # If the server advertises TextDocumentSyncKind::None (0), | |
| # do not send did_change notifications. | |
| next if sync_kind == 0 |
lib/textbringer/commands/lsp.rb
Outdated
| when Hash then sync["change"].to_i | ||
| else 2 |
There was a problem hiding this comment.
If sync is a Hash without a "change" key, sync["change"].to_i evaluates to 0 (None), causing the code to incorrectly send incremental changes (else branch) instead of skipping synchronization. Consider using .dig("change") and providing an explicit default: when Hash then sync.dig("change").to_i.nonzero? || 2 or checking for the presence of the key before calling .to_i.
| if sync_kind == 1 | ||
| # Full document sync (e.g. solargraph) | ||
| client.did_change(uri: uri, version: version, text: buffer.to_s) | ||
| else | ||
| # Deletion: compute old end position from the deleted text | ||
| newline_count = old_text.count("\n") | ||
| if newline_count == 0 | ||
| end_line = start_pos[:line] | ||
| end_char = start_pos[:character] + lsp_utf16_length(old_text) | ||
| # Incremental sync | ||
| # Compute start position in LSP coordinates (0-based, UTF-16) | ||
| start_pos = lsp_position(buffer, beg_pos) | ||
|
|
||
| if old_text.empty? | ||
| # Insertion: old range is empty, new text is the inserted content | ||
| new_text = buffer.substring(beg_pos, end_pos) | ||
| range = { start: start_pos, end: start_pos } | ||
| else | ||
| end_line = start_pos[:line] + newline_count | ||
| last_newline = old_text.rindex("\n") | ||
| end_char = lsp_utf16_length(old_text[last_newline + 1..]) | ||
| # Deletion: compute old end position from the deleted text | ||
| newline_count = old_text.count("\n") | ||
| if newline_count == 0 | ||
| end_line = start_pos[:line] | ||
| end_char = start_pos[:character] + lsp_utf16_length(old_text) | ||
| else | ||
| end_line = start_pos[:line] + newline_count | ||
| last_newline = old_text.rindex("\n") | ||
| end_char = lsp_utf16_length(old_text[last_newline + 1..]) | ||
| end | ||
| range = { | ||
| start: start_pos, | ||
| end: { line: end_line, character: end_char } | ||
| } | ||
| new_text = "" | ||
| end | ||
| range = { | ||
| start: start_pos, | ||
| end: { line: end_line, character: end_char } | ||
| } | ||
| new_text = "" | ||
| end | ||
|
|
||
| client.did_change( | ||
| uri: uri, version: version, | ||
| text: new_text, range: range | ||
| ) | ||
| client.did_change( | ||
| uri: uri, version: version, | ||
| text: new_text, range: range | ||
| ) | ||
| end |
There was a problem hiding this comment.
The code doesn't explicitly handle TextDocumentSyncKind.None (0). If a server specifies sync_kind as 0, the code will still send incremental changes (else branch), which violates the server's expectation. Consider adding a check to skip sending changes when sync_kind is 0: if sync_kind == 0 then next elsif sync_kind == 1 ....
| if sync_kind == 1 | ||
| # Full document sync (e.g. solargraph) | ||
| client.did_change(uri: uri, version: version, text: buffer.to_s) |
There was a problem hiding this comment.
For Full document sync, buffer.to_s is called on every character change, which could have performance implications for large files. Consider adding a note in the comment or documentation about this potential performance impact, especially since this is a significant difference from the previous incremental-only behavior.
- Fix Hash without "change" key: use &.to_i || 2 instead of .to_i (nil.to_i was 0, incorrectly selecting None instead of Incremental) - Handle TextDocumentSyncKind::None (0): skip didChange notifications - Extract lsp_text_document_sync_kind helper for testability - Add 7 tests covering Integer, Hash, missing key, and absent capability Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Solargraph (and other servers) advertise textDocumentSync: 1 (Full), meaning they expect the complete document text on every didChange notification rather than incremental ranges. Sending incremental changes to a Full-sync server leaves it with a stale view of the document, preventing type inference for local variables.
Now lsp_setup_buffer_hooks reads textDocumentSync from server capabilities and sends either a full document or an incremental range accordingly.