Skip to content

Commit 47036d9

Browse files
committed
fix(lsp-server): improve rename feature cache invalidation and test coverage
1 parent c59f81c commit 47036d9

4 files changed

Lines changed: 102 additions & 6 deletions

File tree

docs/specs/beancount-lsp-p0-correctness-implementation-summary-2026-02-25.md

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,56 @@ Validated behaviors in VSCode Web playground (WASM v3):
357357
- message
358358
- source
359359

360+
### G6. Rename cache invalidation follow-up + playground FSA verification (2026-02-27)
361+
362+
Problem observed after the initial P0 rename fixes:
363+
364+
- `rename` can modify unopened files, but server-side caches for those files were not explicitly invalidated in the rename request path.
365+
- `RenameFeature` also queued async reindex work before client-side workspace edits were guaranteed to be applied, which could create short-lived stale index/range behavior.
366+
367+
Fix implemented:
368+
369+
- `packages/lsp-server/src/common/features/rename.ts`
370+
- Added `private invalidateAffectedUris(uris: string[])`
371+
- For every URI in rename results:
372+
- `documents.removeFile(uri)`
373+
- `trees.invalidateCache(uri)`
374+
- Removed rename-time `symbolIndex.addAsyncFile(uri)` enqueueing (reindex now relies on actual document/watch events)
375+
376+
Test coverage added/updated:
377+
378+
- `packages/lsp-server/src/test/features/references-rename.test.ts`
379+
- Added regression test to assert:
380+
- rename invalidates affected document/tree caches
381+
- rename no longer enqueues async reindex directly
382+
- `packages/lsp-server/src/test/utils/test-server-harness.ts`
383+
- Adjusted `InMemoryDocumentStore.removeFile()` semantics to match production `DocumentStore.removeFile()` behavior (clear retrieved-file cache semantics instead of deleting open documents)
384+
385+
Playground browser verification (VSCode Web + FSA mode, Chrome DevTools MCP):
386+
387+
- Repro fixture created in local filesystem (`/tmp/bean-rename-fsa-case`) with:
388+
- `main.bean`
389+
- `a.bean`
390+
- `b.bean`
391+
- Verified scenario:
392+
1. Open `a.bean` only (keep `b.bean` unopened)
393+
2. Rename `Assets:Cash` -> `Assets:Cash:Wallet`
394+
3. Confirmed browser reports `Made 4 text edits in 2 files`
395+
4. Confirmed unopened `b.bean` content was updated on disk
396+
5. Opened `b.bean`, invoked `F2` on renamed token, prepare-rename succeeded
397+
6. Renamed back `Assets:Cash:Wallet` -> `Assets:Cash`
398+
7. Confirmed both files reverted correctly on disk
399+
400+
Validation outcome:
401+
402+
- Cross-file rename on unopened documents works in playground FSA mode.
403+
- No obvious prepare-rename position mismatch was observed on the renamed token after opening `b.bean`.
404+
- `Find References` in this playground repro did not produce usable baseline results (`No references found`), so this iteration did **not** validate post-rename reference-location accuracy in browser UI.
405+
406+
Reviewer note:
407+
408+
- During reverse rename, an editor-side list item (outline/symbol-like UI) briefly appeared stale while on-disk file contents were already correct. This may be a UI refresh lag rather than AST/range corruption, but it remains a good follow-up target if position issues are reported again.
409+
360410
### Review focus
361411

362412
- Confirm config loading remains safe when `getConfiguration` returns non-object values.

packages/lsp-server/src/common/features/rename.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -230,13 +230,19 @@ export class RenameFeature {
230230
Object.keys(changes).length
231231
} files and ${allLocations.length} occurrences (${references.length} references, ${definitions.length} definitions)`,
232232
);
233-
Object.keys(changes).forEach(uri => {
234-
this.symbolIndex.addAsyncFile(uri);
235-
});
233+
this.invalidateAffectedUris(Object.keys(changes));
236234

237235
return { changes };
238236
}
239237

238+
private invalidateAffectedUris(uris: string[]): void {
239+
for (const uri of uris) {
240+
this.documents.removeFile(uri);
241+
this.trees.invalidateCache(uri);
242+
logger.debug(`Invalidated rename-affected caches for ${uri}`);
243+
}
244+
}
245+
240246
private async detectRenameTargetKind(
241247
document: import('vscode-languageserver-textdocument').TextDocument,
242248
position: lsp.Position,

packages/lsp-server/src/test/features/references-rename.test.ts

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ describe('references + rename correctness', () => {
108108
' Income:Salary',
109109
].join('\n');
110110
const docs = new InMemoryDocumentStore({ [uri]: text });
111-
const trees = {} as never;
111+
const trees = { invalidateCache() {} } as never;
112112

113113
const accountDef = makeSymbol(SymbolType.ACCOUNT_DEFINITION, uri, 'Assets:Cash', compact(rangeForOccurrence(text, 'Assets:Cash', 0)));
114114
const accountUse = makeSymbol(SymbolType.ACCOUNT_USAGE, uri, 'Assets:Cash', compact(rangeForOccurrence(text, 'Assets:Cash', 1)));
@@ -120,6 +120,7 @@ describe('references + rename correctness', () => {
120120
const narrationUse = makeSymbol(SymbolType.NARRATION, uri, 'Old Narration', compact(rangeForOccurrence(text, '"Old Narration"', 0)));
121121
const all = [accountDef, accountUse, commodityDef, commodityUse, tagUse, linkUse, payeeUse, narrationUse];
122122

123+
const addAsyncFileSpy = vi.fn();
123124
const symbolIndex = {
124125
async findAsync(query: Record<string, unknown>) {
125126
return all.filter((s) => {
@@ -130,11 +131,12 @@ describe('references + rename correctness', () => {
130131
},
131132
async getAccountDefinitions() { return [accountDef]; },
132133
async getCommodityDefinitions() { return [commodityDef]; },
133-
addAsyncFile() {},
134+
addAsyncFile: addAsyncFileSpy,
134135
} as unknown as import('../../common/features/symbol-index').SymbolIndex;
135136

136137
beforeEach(() => {
137138
positionKinds.clear();
139+
addAsyncFileSpy.mockReset();
138140
});
139141

140142
it('honors includeDeclaration for account references', async () => {
@@ -177,6 +179,42 @@ describe('references + rename correctness', () => {
177179
expect(Object.values(narrationEdit.changes).flat().some((e: any) => e.newText === '"New Narration"')).toBe(true);
178180
});
179181

182+
it('rename invalidates affected document/tree caches and does not enqueue async reindex', async () => {
183+
const secondaryUri = 'file:///secondary.bean';
184+
const removeFileSpy = vi.spyOn(docs, 'removeFile');
185+
const invalidateCacheSpy = vi.fn();
186+
const treesWithInvalidate = { invalidateCache: invalidateCacheSpy } as never;
187+
const rename = new RenameFeature(docs as never, treesWithInvalidate, symbolIndex);
188+
const tagPos = positionAt(text, '#oldtag', 1);
189+
positionKinds.set(posKey(tagPos.line, tagPos.character), 'tag');
190+
191+
const refsSpy = vi.spyOn(ReferencesFeature.prototype, 'onReferences').mockResolvedValue([
192+
{ uri, range: rangeForOccurrence(text, '#oldtag', 0) },
193+
{
194+
uri: secondaryUri,
195+
range: {
196+
start: { line: 0, character: 0 },
197+
end: { line: 0, character: 7 },
198+
},
199+
},
200+
] as any);
201+
const defsSpy = vi.spyOn((rename as any).definitions, 'getDefinition').mockResolvedValue(null);
202+
203+
try {
204+
const edit = await (rename as any).onRename({ textDocument: { uri }, position: tagPos, newName: '#newtag' });
205+
expect(edit).toBeTruthy();
206+
expect(removeFileSpy).toHaveBeenCalledWith(uri);
207+
expect(removeFileSpy).toHaveBeenCalledWith(secondaryUri);
208+
expect(invalidateCacheSpy).toHaveBeenCalledWith(uri);
209+
expect(invalidateCacheSpy).toHaveBeenCalledWith(secondaryUri);
210+
expect(addAsyncFileSpy).not.toHaveBeenCalled();
211+
} finally {
212+
refsSpy.mockRestore();
213+
defsSpy.mockRestore();
214+
removeFileSpy.mockRestore();
215+
}
216+
});
217+
180218
it('rename rejects invalid wrapper-prefixed or malformed names', async () => {
181219
const rename = new RenameFeature(docs as never, trees, symbolIndex);
182220
const tagPos = positionAt(text, '#oldtag', 1);

packages/lsp-server/src/test/utils/test-server-harness.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@ export class InMemoryDocumentStore {
4343
}
4444

4545
removeFile(uri: string): boolean {
46-
return this.docs.delete(uri);
46+
// Mirrors DocumentStore.removeFile semantics: clear only retrieved file cache,
47+
// not LSP-opened documents tracked by TextDocuments.
48+
return this.docs.has(uri);
4749
}
4850

4951
isOpen(uri: string): boolean {

0 commit comments

Comments
 (0)