Merged
Conversation
Reviewer's GuideIntroduce an LSP-side command index and mixin loader so that commands (including those from mixin files) are centrally indexed and available for go-to-definition, while making storage concurrency-safe and wiring indexing into document open/change events. Sequence diagram for LSP document open with indexing and mixin loadingsequenceDiagram
actor Client
participant LSPServer as lspServer
participant Storage as storage
participant Index as index
participant Parser as parser
participant FS as fileSystem
Client->>LSPServer: textDocumentDidOpen(params)
LSPServer->>Storage: AddDocument(uri, text)
activate LSPServer
par async index main document
LSPServer->>Index: IndexDocument(uri, text)
activate Index
Index->>Parser: newParser(log)
Parser-->>Index: parser
Index->>Parser: getCommands(doc)
Parser-->>Index: []command
Index->>Index: rebuild command maps for uri
deactivate Index
and async load mixins
LSPServer->>Storage: GetDocument(uri)
Storage-->>LSPServer: *string doc
LSPServer->>Parser: newParser(log)
Parser-->>LSPServer: parser
LSPServer->>Parser: getMixinFilenames(doc)
Parser-->>LSPServer: []mixinFilename
loop for each mixinFilename
LSPServer->>FS: FileExists(mixinPath)
alt mixin file exists
LSPServer->>FS: ReadFile(mixinPath)
FS-->>LSPServer: data
LSPServer->>Storage: AddDocument(mixinURI, text)
LSPServer->>Index: IndexDocument(mixinURI, text)
else mixin file missing
LSPServer->>LSPServer: log debug missing mixin
end
end
end
deactivate LSPServer
Sequence diagram for go-to-definition using central command indexsequenceDiagram
actor Client
participant LSPServer as lspServer
participant Storage as storage
participant DefHandler as definitionHandler
participant Index as index
Client->>LSPServer: textDocumentDefinition(params)
activate LSPServer
LSPServer->>LSPServer: create definitionHandler(log, parser, index)
LSPServer->>Storage: GetDocument(params.TextDocument.URI)
Storage-->>LSPServer: *string doc
LSPServer->>DefHandler: findCommandDefinition(doc, params)
activate DefHandler
DefHandler->>DefHandler: parser.extractCommandReference(doc, position)
alt command name resolved
DefHandler->>Index: findCommand(commandName)
alt command found in index
Index-->>DefHandler: commandInfo{fileURI, position}
DefHandler->>DefHandler: locationForCommand(fileURI, position)
DefHandler-->>LSPServer: []Location
else command not found
Index-->>DefHandler: not found
DefHandler-->>LSPServer: nil
end
else no command at position
DefHandler-->>LSPServer: nil
end
deactivate DefHandler
LSPServer-->>Client: Location | []Location | nil
deactivate LSPServer
Updated class diagram for LSP server storage, index, and handlersclassDiagram
class lspServer {
-string version
-server.Server server
-storage storage
-index index
-commonlog.Logger log
+Run() error
+textDocumentDidOpen(context *glsp.Context, params *lsp.DidOpenTextDocumentParams) error
+textDocumentDidChange(context *glsp.Context, params *lsp.DidChangeTextDocumentParams) error
+textDocumentDefinition(context *glsp.Context, params *lsp.DefinitionParams) (any, error)
+textDocumentCompletion(context *glsp.Context, params *lsp.CompletionParams) (any, error)
-loadMixins(uri string)
}
class storage {
-sync.RWMutex mu
-map~string,*string~ documents
+GetDocument(uri string) *string
+AddDocument(uri string, text string)
}
class index {
-commonlog.Logger log
-sync.RWMutex mu
-map~string,commandInfo~ commands
-map~string,set.Set~string~~ commandsByURI
+IndexDocument(uri string, doc string)
+findCommand(name string) (commandInfo, bool)
}
class commandInfo {
-string fileURI
-lsp.Position position
}
class definitionHandler {
-commonlog.Logger log
-parser parser
-index index
+findMixinsDefinition(doc *string, params *lsp.DefinitionParams) (any, error)
+findCommandDefinition(doc *string, params *lsp.DefinitionParams) (any, error)
}
class parser {
-commonlog.Logger log
+getCommands(document *string) []command
+getMixinFilenames(document *string) []string
+extractCommandReference(document *string, position lsp.Position) string
}
class command {
-string name
-lsp.Position position
}
lspServer --> storage : uses
lspServer --> index : uses
lspServer --> parser : creates
lspServer --> definitionHandler : creates
definitionHandler --> parser : uses
definitionHandler --> index : uses
definitionHandler --> commandInfo : returns
index "1" --> "*" commandInfo : stores
index --> parser : creates
storage "1" --> "*" string : stores documents
parser "1" --> "*" command : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
IndexDocumentmethod creates a new parser on every call; if this is called frequently (e.g., on every change), consider reusing a shared parser instance on theindexto avoid repeated allocations and parser setup cost. - The
getMixinFilenamesYAML query only matches mixin values as plain scalars in a block sequence; if you expect mixins to be specified using other YAML forms (e.g., quoted strings or flow sequences), you may want to extend the query to handle those node shapes as well. - Because
loadMixinsis called from goroutines and performs filesystem I/O, you might want to consider adding basic error/log context (e.g., including the parent document URI) or rate-limiting/debouncing if many documents are opened rapidly to prevent excessive concurrent reads.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `IndexDocument` method creates a new parser on every call; if this is called frequently (e.g., on every change), consider reusing a shared parser instance on the `index` to avoid repeated allocations and parser setup cost.
- The `getMixinFilenames` YAML query only matches mixin values as plain scalars in a block sequence; if you expect mixins to be specified using other YAML forms (e.g., quoted strings or flow sequences), you may want to extend the query to handle those node shapes as well.
- Because `loadMixins` is called from goroutines and performs filesystem I/O, you might want to consider adding basic error/log context (e.g., including the parent document URI) or rate-limiting/debouncing if many documents are opened rapidly to prevent excessive concurrent reads.
## Individual Comments
### Comment 1
<location path="internal/lsp/index.go" line_range="59" />
<code_context>
+ delete(i.commands, name)
+ }
+
+ maps.Copy(i.commands, indexedCommands)
+
+ if len(indexedNames) == 0 {
</code_context>
<issue_to_address>
**issue (bug_risk):** Indexing logic breaks when the same command name exists in multiple files.
This relies on command names being globally unique. With two files defining the same command, `maps.Copy` overwrites `commands[name]` while `commandsByURI` for the original file still lists that command. On reindexing the first file, the cleanup loop deletes `commands[name]`, removing the entry that actually belongs to the second file.
If multiple files can legitimately define the same command, the index should be keyed by `(uri, name)` or similar. If not, we should explicitly enforce/document global uniqueness and guard against duplicates rather than silently corrupting the index state.
</issue_to_address>
### Comment 2
<location path="internal/lsp/handlers.go" line_range="48-57" />
<code_context>
}
+// loadMixins reads local mixin files referenced by a document and adds them to storage and index.
+func (s *lspServer) loadMixins(uri string) {
+ doc := s.storage.GetDocument(uri)
+ if doc == nil {
+ return
+ }
+
+ parser := newParser(s.log)
+ path := normalizePath(uri)
+
+ for _, filename := range parser.getMixinFilenames(doc) {
+ mixinPath := replacePathFilename(path, strings.TrimPrefix(filename, "-"))
+ if !util.FileExists(mixinPath) {
+ s.log.Debugf("mixin target does not exist: %s", mixinPath)
+ continue
+ }
+
+ data, err := os.ReadFile(mixinPath)
+ if err != nil {
+ s.log.Warningf("failed to read mixin %s: %v", mixinPath, err)
+ continue
+ }
+
+ mixinURI := pathToURI(mixinPath)
+ text := string(data)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Loaded mixin documents are never removed from storage/index when references or files disappear.
This helper only adds mixin documents to storage/index, but never removes them when a mixin file is deleted/moved or when a document drops a mixin reference. That can leave unreachable mixin documents and stale definitions in the index. Please add a mechanism to track mixins per document and remove them from storage and the index when they’re no longer referenced (or when the document is closed).
</issue_to_address>
### Comment 3
<location path="internal/lsp/index_test.go" line_range="8" />
<code_context>
+ "testing"
+)
+
+func TestIndexDocumentStoresCommands(t *testing.T) {
+ doc := `commands:
+ build:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test that covers indexing multiple documents and reindexing only one of them
The existing tests only cover a single-URI case. Because `commandsByURI` tracks commands per document, a test should verify that updating one URI doesn’t affect commands for another.
For example:
- Index `file:///a.yaml` with `build`.
- Index `file:///b.yaml` with `test`.
- Reindex only `file:///a.yaml`, changing `build` → `release`.
- Assert:
- `findCommand("release")` points to `a.yaml`.
- `findCommand("test")` still exists and points to `b.yaml`.
This guards against mistakenly removing commands for other URIs when cleaning up a single document’s entries.
Suggested implementation:
```golang
import (
"reflect"
"testing"
)
func TestIndexDocument_ReindexSingleDocumentDoesNotAffectOthers(t *testing.T) {
idx := newIndex(logger)
docAInitial := `commands:
build:
cmd: echo build`
docB := `commands:
test:
cmd: echo test`
// Index two different URIs
idx.IndexDocument("file:///a.yaml", docAInitial)
idx.IndexDocument("file:///b.yaml", docB)
// Reindex only a.yaml, changing build -> release
docAUpdated := `commands:
release:
cmd: echo release`
idx.IndexDocument("file:///a.yaml", docAUpdated)
// release should now exist and point to a.yaml
releaseCmd, ok := idx.findCommand("release")
if !ok {
t.Fatalf("expected command %q to exist after reindex", "release")
}
if releaseCmd.uri != "file:///a.yaml" {
t.Fatalf("expected %q to be defined in %q, got %q", "release", "file:///a.yaml", releaseCmd.uri)
}
// test should still exist and point to b.yaml
testCmd, ok := idx.findCommand("test")
if !ok {
t.Fatalf("expected command %q to still exist after reindex of another document", "test")
}
if testCmd.uri != "file:///b.yaml" {
t.Fatalf("expected %q to be defined in %q, got %q", "test", "file:///b.yaml", testCmd.uri)
}
// build should no longer exist
if _, ok := idx.findCommand("build"); ok {
t.Fatalf("expected command %q to be removed after reindex of its document", "build")
}
}
```
Depending on the existing index implementation, you may need to adjust the test slightly:
1. **`findCommand` signature**
- If `findCommand` is not a method on `idx` but a free function, call it accordingly (e.g. `findCommand(idx, "release")` or similar).
- If it returns `(commandInfo, bool)` with a different name or structure, update the variable names and assertion logic.
2. **`commandInfo` fields**
- This test assumes `commandInfo` has a field named `uri` containing the document URI.
- If the field is named differently (e.g. `URI`, `Uri`, or `documentURI`), update `releaseCmd.uri`, `testCmd.uri` accordingly.
3. **Indexing API differences**
- If `IndexDocument` has a different signature, adjust the calls to match (e.g. context, version, or additional parameters).
The core behavior to preserve is:
- Two distinct URIs are indexed with different commands.
- Reindexing only one URI replaces its commands without affecting the other URI’s commands.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| delete(i.commands, name) | ||
| } | ||
|
|
||
| maps.Copy(i.commands, indexedCommands) |
There was a problem hiding this comment.
issue (bug_risk): Indexing logic breaks when the same command name exists in multiple files.
This relies on command names being globally unique. With two files defining the same command, maps.Copy overwrites commands[name] while commandsByURI for the original file still lists that command. On reindexing the first file, the cleanup loop deletes commands[name], removing the entry that actually belongs to the second file.
If multiple files can legitimately define the same command, the index should be keyed by (uri, name) or similar. If not, we should explicitly enforce/document global uniqueness and guard against duplicates rather than silently corrupting the index state.
internal/lsp/handlers.go
Outdated
Comment on lines
+48
to
+57
| func (s *lspServer) loadMixins(uri string) { | ||
| doc := s.storage.GetDocument(uri) | ||
| if doc == nil { | ||
| return | ||
| } | ||
|
|
||
| parser := newParser(s.log) | ||
| path := normalizePath(uri) | ||
|
|
||
| for _, filename := range parser.getMixinFilenames(doc) { |
There was a problem hiding this comment.
issue (bug_risk): Loaded mixin documents are never removed from storage/index when references or files disappear.
This helper only adds mixin documents to storage/index, but never removes them when a mixin file is deleted/moved or when a document drops a mixin reference. That can leave unreachable mixin documents and stale definitions in the index. Please add a mechanism to track mixins per document and remove them from storage and the index when they’re no longer referenced (or when the document is closed).
| "testing" | ||
| ) | ||
|
|
||
| func TestIndexDocumentStoresCommands(t *testing.T) { |
There was a problem hiding this comment.
suggestion (testing): Consider a test that covers indexing multiple documents and reindexing only one of them
The existing tests only cover a single-URI case. Because commandsByURI tracks commands per document, a test should verify that updating one URI doesn’t affect commands for another.
For example:
- Index
file:///a.yamlwithbuild. - Index
file:///b.yamlwithtest. - Reindex only
file:///a.yaml, changingbuild→release. - Assert:
findCommand("release")points toa.yaml.findCommand("test")still exists and points tob.yaml.
This guards against mistakenly removing commands for other URIs when cleaning up a single document’s entries.
Suggested implementation:
import (
"reflect"
"testing"
)
func TestIndexDocument_ReindexSingleDocumentDoesNotAffectOthers(t *testing.T) {
idx := newIndex(logger)
docAInitial := `commands:
build:
cmd: echo build`
docB := `commands:
test:
cmd: echo test`
// Index two different URIs
idx.IndexDocument("file:///a.yaml", docAInitial)
idx.IndexDocument("file:///b.yaml", docB)
// Reindex only a.yaml, changing build -> release
docAUpdated := `commands:
release:
cmd: echo release`
idx.IndexDocument("file:///a.yaml", docAUpdated)
// release should now exist and point to a.yaml
releaseCmd, ok := idx.findCommand("release")
if !ok {
t.Fatalf("expected command %q to exist after reindex", "release")
}
if releaseCmd.uri != "file:///a.yaml" {
t.Fatalf("expected %q to be defined in %q, got %q", "release", "file:///a.yaml", releaseCmd.uri)
}
// test should still exist and point to b.yaml
testCmd, ok := idx.findCommand("test")
if !ok {
t.Fatalf("expected command %q to still exist after reindex of another document", "test")
}
if testCmd.uri != "file:///b.yaml" {
t.Fatalf("expected %q to be defined in %q, got %q", "test", "file:///b.yaml", testCmd.uri)
}
// build should no longer exist
if _, ok := idx.findCommand("build"); ok {
t.Fatalf("expected command %q to be removed after reindex of its document", "build")
}
}Depending on the existing index implementation, you may need to adjust the test slightly:
-
findCommandsignature- If
findCommandis not a method onidxbut a free function, call it accordingly (e.g.findCommand(idx, "release")or similar). - If it returns
(commandInfo, bool)with a different name or structure, update the variable names and assertion logic.
- If
-
commandInfofields- This test assumes
commandInfohas a field nameduricontaining the document URI. - If the field is named differently (e.g.
URI,Uri, ordocumentURI), updatereleaseCmd.uri,testCmd.uriaccordingly.
- This test assumes
-
Indexing API differences
- If
IndexDocumenthas a different signature, adjust the calls to match (e.g. context, version, or additional parameters).
- If
The core behavior to preserve is:
- Two distinct URIs are indexed with different commands.
- Reindexing only one URI replaces its commands without affecting the other URI’s commands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Index LSP documents and their mixin commands to support cross-file command navigation.
New Features:
Enhancements:
Documentation:
Tests: