Skip to content

Optimize index updates #332

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.

Sign up for GitHub

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

Merged
kindermax merged 2 commits into from
Apr 3, 2026
+221 −7
Merged

Optimize index updates #332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a13fc3f
Debounde index updates
kindermax Apr 2, 2026
2aa4804
Optimize LSP mixin loading and updates
kindermax Apr 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions internal/lsp/debounce.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package lsp

import (
"sync"
"time"
)

type documentDebouncer struct {
delay time.Duration
refresh func(string)

mu sync.Mutex
timers map[string]*time.Timer
}

func newDocumentDebouncer(delay time.Duration, refresh func(string)) *documentDebouncer {
return &documentDebouncer{
delay: delay,
refresh: refresh,
timers: make(map[string]*time.Timer),
}
}

func (d *documentDebouncer) Schedule(uri string) {
d.mu.Lock()
defer d.mu.Unlock()

if timer, ok := d.timers[uri]; ok {
timer.Stop()
}

var timer *time.Timer

timer = time.AfterFunc(d.delay, func() {
d.fire(uri, timer)
})

d.timers[uri] = timer
}

func (d *documentDebouncer) Stop() {
d.mu.Lock()
defer d.mu.Unlock()

for uri, timer := range d.timers {
timer.Stop()
delete(d.timers, uri)
}
}

func (d *documentDebouncer) fire(uri string, timer *time.Timer) {
d.mu.Lock()

current, ok := d.timers[uri]
if !ok || current != timer {
d.mu.Unlock()
return
}

delete(d.timers, uri)
d.mu.Unlock()

d.refresh(uri)
}
91 changes: 91 additions & 0 deletions internal/lsp/debounce_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package lsp

import (
"testing"
"time"

"github.com/tliron/glsp"
lsp "github.com/tliron/glsp/protocol_3_16"
)

func TestDocumentDebouncerCoalescesRepeatedSchedules(t *testing.T) {
events := make(chan string, 2)
debouncer := newDocumentDebouncer(20*time.Millisecond, func(uri string) {
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Apr 3, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Consider making the debounce timing assertions more robust to avoid flaky tests

These tests depend on tight, hard-coded timing (20ms delay with 60–300ms timeouts), which can be fragile on slower CI runners. To make them more resilient, consider: (1) increasing the timeout safety margin (e.g., 5–10× the debounce delay), and/or (2) extracting the timing values into constants so they can be adjusted centrally if flakes occur.

Suggested implementation:

import (
	"testing"
	"time"

	"github.com/tliron/glsp"
	lsp "github.com/tliron/glsp/protocol_3_16"
)

const (
	// testDebounceDelay is the debounce delay used for tests.
	testDebounceDelay = 20 * time.Millisecond

	// testDebounceTimeoutShort is a generous timeout (10× the debounce delay)
	// used for assertions that a debounced event *should* have fired.
	testDebounceTimeoutShort = 10 * testDebounceDelay

	// testDebounceTimeoutLong is a longer timeout used where the test logic
	// needs more time; keeping it a named constant allows easy adjustment
	// if CI flakes occur.
	testDebounceTimeoutLong = 15 * testDebounceDelay
)
func TestDocumentDebouncerCoalescesRepeatedSchedules(t *testing.T) {
	events := make(chan string, 2)
	debouncer := newDocumentDebouncer(testDebounceDelay, func(uri string) {
		events <- uri
	})
	defer debouncer.Stop()

I only see part of the file. To fully apply the suggestion, you should:

  1. Replace any hard-coded timeouts like time.After(60 * time.Millisecond) with time.After(testDebounceTimeoutShort).
  2. Replace any longer hard-coded timeouts such as time.After(300 * time.Millisecond) (or similar) with time.After(testDebounceTimeoutLong) or introduce additional constants if you need more distinct timeout categories.
  3. Replace any other newDocumentDebouncer(<literal> * time.Millisecond, ...) calls with newDocumentDebouncer(testDebounceDelay, ...) unless a test intentionally uses a different delay.

This will centralize timing configuration and make it easy to tweak if you ever see CI flakes.

events <- uri
})
defer debouncer.Stop()

debouncer.Schedule("file:///tmp/lets.yaml")
debouncer.Schedule("file:///tmp/lets.yaml")

select {
case got := <-events:
if got != "file:///tmp/lets.yaml" {
t.Fatalf("refresh uri = %q, want %q", got, "file:///tmp/lets.yaml")
}
case <-time.After(200 * time.Millisecond):
t.Fatal("timed out waiting for debounced refresh")
}

select {
case got := <-events:
t.Fatalf("unexpected extra refresh for %q", got)
case <-time.After(60 * time.Millisecond):
}
}

func TestTextDocumentDidChangeUsesLatestDocumentAfterDebounce(t *testing.T) {
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Apr 3, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Add a test that verifies no further refreshes occur after the debouncer is stopped (e.g. during shutdown)

Given the new Stop() on documentDebouncer and its use during shutdown, please also add a test that:

  • schedules a refresh,
  • calls Stop() (or simulates shutdown),
  • waits longer than the debounce delay,
  • and asserts the refresh callback is never invoked.

This will verify that in-flight timers are cancelled and no index updates occur after shutdown.

server := &lspServer{
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot Apr 3, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Add tests that cover mixin-related behavior with the new debounced refresh flow

This test covers textDocumentDidChange focusing on command indexing, but the new refreshDocument flow also calls loadMixins, which now tracks previous vs current mixins and skips already-managed documents. Please add an integration-style test that changes a document’s mixins over time and asserts that:

  • newly added mixins are loaded,
  • previously used mixins are not redundantly reloaded, and
  • mixins already in storage are skipped.
    This will directly exercise the optimized index/mixin update path and guard against regressions in mixin handling.

Suggested implementation:

	}
}

type trackingStorage struct {
	Storage interface {
		// Adjust these methods to match the actual storage interface.
		// The important part is that this wrapper is used by lspServer so we can
		// count how often mixin documents are loaded.
		SaveDocument(ctx context.Context, uri lsp.DocumentURI, text string) error
		LoadDocument(ctx context.Context, uri lsp.DocumentURI) (string, error)
	}
	loadCounts map[lsp.DocumentURI]int
}

func newTrackingStorage(underlying interface {
	SaveDocument(ctx context.Context, uri lsp.DocumentURI, text string) error
	LoadDocument(ctx context.Context, uri lsp.DocumentURI) (string, error)
}) *trackingStorage {
	return &trackingStorage{
		Storage:    underlying,
		loadCounts: make(map[lsp.DocumentURI]int),
	}
}

func (s *trackingStorage) SaveDocument(ctx context.Context, uri lsp.DocumentURI, text string) error {
	// Pass-through; we don't need to track writes for this test.
	return s.Storage.SaveDocument(ctx, uri, text)
}

func (s *trackingStorage) LoadDocument(ctx context.Context, uri lsp.DocumentURI) (string, error) {
	s.loadCounts[uri]++
	return s.Storage.LoadDocument(ctx, uri)
}

func TestRefreshDocumentDebouncedMixins(t *testing.T) {
	logger := logger // reuse the package-level test logger if available

	baseStorage := newStorage()
	tracking := newTrackingStorage(baseStorage)

	server := &lspServer{
		// NOTE: lspServer.storage must be compatible with trackingStorage's interface.
		storage: tracking,
		parser:  newParser(logger),
		index:   newIndex(logger),
		log:     logger,
	}
	server.refresh = newDocumentDebouncer(20*time.Millisecond, server.refreshDocument)
	defer server.refresh.Stop()

	ctx := context.Background()
	mainURI := lsp.DocumentURI("file:///tmp/lets.yaml")
	mixin1 := lsp.DocumentURI("file:///tmp/mixin1.lets")
	mixin2 := lsp.DocumentURI("file:///tmp/mixin2.lets")
	mixin3 := lsp.DocumentURI("file:///tmp/mixin3.lets")

	// Helper to trigger a debounced refresh via textDocument/didChange using the
	// same entrypoint as the other tests.
	changeMain := func(text string) {
		params := &lsp.DidChangeTextDocumentParams{
			TextDocument: lsp.VersionedTextDocumentIdentifier{
				TextDocumentIdentifier: lsp.TextDocumentIdentifier{URI: mainURI},
			},
			ContentChanges: []lsp.TextDocumentContentChangeEvent{
				{Text: text},
			},
		}
		if err := server.textDocumentDidChange(ctx, params); err != nil {
			t.Fatalf("textDocumentDidChange: %v", err)
		}
		time.Sleep(50 * time.Millisecond) // wait for debounce to fire
	}

	// 1) Start with a document that uses mixin1.
	changeMain("uses:\n  - " + string(mixin1) + "\n")

	if got := tracking.loadCounts[mixin1]; got != 1 {
		t.Fatalf("expected mixin1 to be loaded once on initial refresh, got %d", got)
	}
	if got := tracking.loadCounts[mixin2]; got != 0 {
		t.Fatalf("expected mixin2 to not be loaded yet, got %d", got)
	}
	if got := tracking.loadCounts[mixin3]; got != 0 {
		t.Fatalf("expected mixin3 to not be loaded yet, got %d", got)
	}

	// 2) Change the document to still use mixin1 and also use mixin2.
	changeMain("uses:\n  - " + string(mixin1) + "\n  - " + string(mixin2) + "\n")

	// mixin1 is still in use but should not be redundantly reloaded.
	if got := tracking.loadCounts[mixin1]; got != 1 {
		t.Fatalf("expected mixin1 to not be reloaded, still 1 load; got %d", got)
	}
	// mixin2 is newly added and should be loaded exactly once.
	if got := tracking.loadCounts[mixin2]; got != 1 {
		t.Fatalf("expected mixin2 to be loaded once after being added, got %d", got)
	}

	// 3) Pre-load mixin3 into storage to simulate it already being managed.
	if err := tracking.SaveDocument(ctx, mixin3, "command: mixin3"); err != nil {
		t.Fatalf("preloading mixin3 into storage: %v", err)
	}

	// Now change the document to use mixin1, mixin2, and mixin3.
	changeMain("uses:\n  - " + string(mixin1) + "\n  - " + string(mixin2) + "\n  - " + string(mixin3) + "\n")

	// mixin1 and mixin2 are still in use; no extra loads.
	if got := tracking.loadCounts[mixin1]; got != 1 {
		t.Fatalf("expected mixin1 to still have exactly 1 load, got %d", got)
	}
	if got := tracking.loadCounts[mixin2]; got != 1 {
		t.Fatalf("expected mixin2 to still have exactly 1 load, got %d", got)
	}

	// mixin3 was already present in storage and should be skipped by loadMixins.
	if got := tracking.loadCounts[mixin3]; got != 0 {
		t.Fatalf("expected mixin3 to be skipped because it is already in storage, got %d loads", got)
	}
}

func TestTextDocumentDidChangeUsesLatestDocumentAfterDebounce(t *testing.T) {
	server := &lspServer{
		storage: newStorage(),
		parser:  newParser(logger),
		index:   newIndex(logger),
		log:     logger,
	}
	server.refresh = newDocumentDebouncer(20*time.Millisecond, server.refreshDocument)
	defer server.refresh.Stop()

	params := &lsp.DidChangeTextDocumentParams{
		TextDocument: lsp.VersionedTextDocumentIdentifier{
			TextDocumentIdentifier: lsp.TextDocumentIdentifier{URI: "file:///tmp/lets.yaml"},
		},

The above changes assume certain shapes of the storage and server APIs that may differ slightly from your actual code:

  1. storage abstraction

    • Replace the trackingStorage interface methods (SaveDocument, LoadDocument) with the actual methods your lspServer calls during refreshDocument / loadMixins.
    • If your concrete storage type is exported (e.g. type storage struct { ... }), make trackingStorage embed it (e.g. type trackingStorage struct { *storage; loadCounts map[...]int }) and override only the load method that loadMixins uses to fetch mixin documents.

  2. URIs and mixin syntax

    • Adjust the mixin1, mixin2, and mixin3 URIs and the changeMain text payloads to match how your parser detects mixins (e.g. different YAML keys, relative paths, etc.). The test is structured so that:
      • first change → mixin1 only,
      • second change → mixin1 + mixin2,
      • third change → mixin1 + mixin2 + mixin3 (already in storage).

  3. Debouncer triggering

    • Replace the time.Sleep(50 * time.Millisecond) with the same synchronization strategy used by your existing debounce tests (e.g. reading from a channel of refresh events if the debouncer exposes one, or using a slightly longer sleep consistent with your other tests).

  4. Error handling

    • If textDocumentDidChange has a different signature (e.g. doesn’t return an error), remove the error check accordingly.

Once you align trackingStorage with your actual storage API and tweak the URI/mixin syntax to what your parser expects, this test will directly exercise the debounced refreshDocument + loadMixins path and verify that:

  • new mixins are loaded,
  • existing-but-still-used mixins are not redundantly reloaded, and
  • already-stored mixins are skipped.

storage: newStorage(),
parser: newParser(logger),
index: newIndex(logger),
log: logger,
}
server.refresh = newDocumentDebouncer(20*time.Millisecond, server.refreshDocument)
defer server.refresh.Stop()

params := &lsp.DidChangeTextDocumentParams{
TextDocument: lsp.VersionedTextDocumentIdentifier{
TextDocumentIdentifier: lsp.TextDocumentIdentifier{URI: "file:///tmp/lets.yaml"},
},
}

params.ContentChanges = []any{
lsp.TextDocumentContentChangeEventWhole{
Text: `commands:
build:
cmd: echo build`,
},
}

if err := server.textDocumentDidChange(&glsp.Context{}, params); err != nil {
t.Fatalf("first textDocumentDidChange() error = %v", err)
}

params.ContentChanges = []any{
lsp.TextDocumentContentChangeEventWhole{
Text: `commands:
release:
cmd: echo release`,
},
}

if err := server.textDocumentDidChange(&glsp.Context{}, params); err != nil {
t.Fatalf("second textDocumentDidChange() error = %v", err)
}

deadline := time.Now().Add(300 * time.Millisecond)
for time.Now().Before(deadline) {
if _, ok := server.index.findCommand("release"); ok {
if _, ok := server.index.findCommand("build"); ok {
t.Fatal("expected stale command to be removed after debounced refresh")
}

return
}

time.Sleep(10 * time.Millisecond)
}

t.Fatal("timed out waiting for debounced document refresh")
}
47 changes: 41 additions & 6 deletions internal/lsp/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ func (s *lspServer) initialized(context *glsp.Context, params *lsp.InitializedPa
}

func (s *lspServer) shutdown(context *glsp.Context) error {
if s.refresh != nil {
s.refresh.Stop()
}

lsp.SetTraceValue(lsp.TraceValueOff)

return nil
}

Expand All @@ -53,8 +58,25 @@ func (s *lspServer) loadMixins(uri string) {

path := normalizePath(uri)

for _, filename := range s.parser.getMixinFilenames(doc) {
currentMixins := s.parser.getMixinFilenames(doc)
previousMixins := s.storage.GetMixins(uri)

s.storage.SetMixins(uri, currentMixins)

for _, filename := range currentMixins {
if slices.Contains(previousMixins, filename) {
continue
}

mixinPath := replacePathFilename(path, strings.TrimPrefix(filename, "-"))
mixinURI := pathToURI(mixinPath)

// skip if the document is already managed by the storage
// e.g. opened manually in the editor or already loaded
if s.storage.GetDocument(mixinURI) != nil {
continue
}

if !util.FileExists(mixinPath) {
s.log.Debugf("mixin target does not exist: %s", mixinPath)
continue
Expand All @@ -66,19 +88,29 @@ func (s *lspServer) loadMixins(uri string) {
continue
}

mixinURI := pathToURI(mixinPath)
s.log.Debugf("load mixin %s", mixinPath)

text := string(data)

s.storage.AddDocument(mixinURI, text)
s.index.IndexDocument(mixinURI, text)
}
}

func (s *lspServer) refreshDocument(uri string) {
doc := s.storage.GetDocument(uri)
if doc == nil {
return
}

s.index.IndexDocument(uri, *doc)
s.loadMixins(uri)
}

func (s *lspServer) textDocumentDidOpen(context *glsp.Context, params *lsp.DidOpenTextDocumentParams) error {
s.storage.AddDocument(params.TextDocument.URI, params.TextDocument.Text)

go s.index.IndexDocument(params.TextDocument.URI, params.TextDocument.Text)
go s.loadMixins(params.TextDocument.URI)
go s.refreshDocument(params.TextDocument.URI)

return nil
}
Expand All @@ -89,8 +121,11 @@ func (s *lspServer) textDocumentDidChange(context *glsp.Context, params *lsp.Did
case lsp.TextDocumentContentChangeEventWhole:
s.storage.AddDocument(params.TextDocument.URI, c.Text)

go s.index.IndexDocument(params.TextDocument.URI, c.Text)
go s.loadMixins(params.TextDocument.URI)
if s.refresh != nil {
s.refresh.Schedule(params.TextDocument.URI)
} else {
go s.refreshDocument(params.TextDocument.URI)
}
case lsp.TextDocumentContentChangeEvent:
return errors.New("incremental changes not supported")
}
Expand Down
8 changes: 7 additions & 1 deletion internal/lsp/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package lsp

import (
"context"
"time"

"github.com/lets-cli/lets/internal/env"
"github.com/tliron/commonlog"
Expand All @@ -10,7 +11,10 @@ import (
"github.com/tliron/glsp/server"
)

const lsName = "lets_ls"
const (
lsName = "lets_ls"
documentRefreshDebounce = 500 * time.Millisecond
)

var handler lsp.Handler

Expand All @@ -20,6 +24,7 @@ type lspServer struct {
storage *storage
parser *parser
index *index
refresh *documentDebouncer
log commonlog.Logger
}

Expand Down Expand Up @@ -60,6 +65,7 @@ func Run(ctx context.Context, version string) error {
index: newIndex(logger),
log: logger,
}
lspServer.refresh = newDocumentDebouncer(documentRefreshDebounce, lspServer.refreshDocument)

handler.Initialize = lspServer.initialize
handler.Initialized = lspServer.initialized
Expand Down
16 changes: 16 additions & 0 deletions internal/lsp/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import "sync"
type storage struct {
mu sync.RWMutex
documents map[string]*string
mixins map[string][]string
}

func newStorage() *storage {
return &storage{
documents: make(map[string]*string),
mixins: make(map[string][]string),
}
}

Expand All @@ -26,3 +28,17 @@ func (s *storage) AddDocument(uri string, text string) {

s.documents[uri] = &text
}

func (s *storage) SetMixins(uri string, mixins []string) {
s.mu.Lock()
defer s.mu.Unlock()

s.mixins[uri] = mixins
}

func (s *storage) GetMixins(uri string) []string {
s.mu.RLock()
defer s.mu.RUnlock()

return s.mixins[uri]
}
2 changes: 2 additions & 0 deletions internal/settings/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,10 @@ func LoadFile(path string) (Settings, error) {
defer file.Close()

var fileSettings FileSettings

decoder := yaml.NewDecoder(file)
decoder.KnownFields(true)

if err := decoder.Decode(&fileSettings); err != nil {
return Settings{}, fmt.Errorf("failed to decode settings file: %w", err)
}
Expand Down
Loading