Skip to content

Commit fa5864a

Browse files
atennak1atennak1
authored
fix handling of multiple sequential edits (#292)
1 parent a51a312 commit fa5864a

File tree

7 files changed

+740
-107
lines changed

7 files changed

+740
-107
lines changed

‎src/context/syntaxtree/SyntaxTree.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -779,6 +779,10 @@ export abstract class SyntaxTree {
779779
return this.tree.rootNode.text;
780780
}
781781

782+
public getRootNode() {
783+
return this.tree.rootNode;
784+
}
785+
782786
/**
783787
* Find block_mapping that contains keys at the same indentation level as the cursor
784788
* Used for creating sibling synthetic keys

‎src/handlers/DocumentHandler.ts‎

Lines changed: 78 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
1-
import { Edit, Point } from 'tree-sitter';
1+
import { Point } from 'tree-sitter';
22
import { DidChangeTextDocumentParams } from 'vscode-languageserver';
33
import { TextDocumentChangeEvent } from 'vscode-languageserver/lib/common/textDocuments';
44
import { NotificationHandler } from 'vscode-languageserver-protocol';
55
import { TextDocument } from 'vscode-languageserver-textdocument';
6-
import { SyntaxTreeManager } from '../context/syntaxtree/SyntaxTreeManager';
76
import { CloudFormationFileType, Document } from '../document/Document';
87
import { createEdit } from '../document/DocumentUtils';
98
import { LspDocuments } from '../protocol/LspDocuments';
@@ -35,26 +34,9 @@ export function didOpenHandler(components: ServerComponents): (event: TextDocume
3534
}
3635
}
3736

38-
components.cfnLintService.lintDelayed(content, uri, LintTrigger.OnOpen).catch((reason) => {
39-
// Handle cancellation gracefully - user might have closed/changed the document
40-
if (reason instanceof CancellationError) {
41-
// Do nothing - cancellation is expected behavior
42-
} else {
43-
log.error(reason, `Linting error for ${uri}`);
44-
}
45-
});
37+
triggerValidation(components, content, uri, LintTrigger.OnOpen, ValidationTrigger.OnOpen);
4638

47-
// Trigger Guard validation
48-
components.guardService.validateDelayed(content, uri, ValidationTrigger.OnOpen).catch((reason) => {
49-
// Handle cancellation gracefully - user might have closed/changed the document
50-
if (reason instanceof Error && reason.message.includes('Request cancelled')) {
51-
// Do nothing
52-
} else {
53-
log.error(reason, `Guard validation error for ${uri}`);
54-
}
55-
});
56-
57-
components.documentManager.sendDocumentMetadata(0);
39+
components.documentManager.sendDocumentMetadata();
5840
};
5941
}
6042

@@ -64,66 +46,72 @@ export function didChangeHandler(
6446
): NotificationHandler<DidChangeTextDocumentParams> {
6547
return (params) => {
6648
const documentUri = params.textDocument.uri;
49+
const version = params.textDocument.version;
6750
const textDocument = documents.documents.get(documentUri);
6851

6952
if (!textDocument) {
7053
log.error(`No document found for file with changes ${documentUri}`);
7154
return;
7255
}
7356

74-
const content = textDocument.getText();
75-
const changes = params.contentChanges;
76-
try {
77-
let hasFullDocumentChange = false;
78-
for (const change of changes) {
79-
if ('range' in change) {
80-
// This is an incremental change with a specific range
81-
const start: Point = {
82-
row: change.range.start.line,
83-
column: change.range.start.character,
84-
};
85-
const end: Point = {
86-
row: change.range.end.line,
87-
column: change.range.end.character,
88-
};
89-
90-
const { edit } = createEdit(content, change.text, start, end);
91-
updateSyntaxTree(components.syntaxTreeManager, textDocument, edit);
92-
} else {
93-
hasFullDocumentChange = true;
94-
}
95-
}
57+
// This is the document AFTER changes
58+
const document = new Document(textDocument);
59+
const finalContent = document.getText();
9660

97-
if (hasFullDocumentChange) {
98-
components.syntaxTreeManager.add(documentUri, content);
61+
const tree = components.syntaxTreeManager.getSyntaxTree(documentUri);
62+
63+
// Short-circuit if this is not a template (anymore)
64+
if (document.cfnFileType === CloudFormationFileType.Other) {
65+
if (tree) {
66+
// Clean-up if was but no longer is a template
67+
components.syntaxTreeManager.deleteSyntaxTree(documentUri);
9968
}
100-
} catch (error) {
101-
log.error(error, `Error updating tree ${documentUri}`);
102-
// Create a new tree if partial updates fail
103-
components.syntaxTreeManager.add(documentUri, content);
69+
components.documentManager.sendDocumentMetadata();
70+
return;
10471
}
10572

106-
// Trigger cfn-lint validation
107-
components.cfnLintService.lintDelayed(content, documentUri, LintTrigger.OnChange, true).catch((reason) => {
108-
// Handle both getTextDocument and linting errors
109-
if (reason instanceof CancellationError) {
110-
// Do nothing - cancellation is expected behavior
111-
} else {
112-
log.error(reason, `Error in didChange processing for ${documentUri}`);
73+
if (tree) {
74+
// This starts as the text BEFORE changes
75+
let currentContent = tree.content();
76+
try {
77+
const changes = params.contentChanges;
78+
for (const change of changes) {
79+
if ('range' in change) {
80+
// Incremental change
81+
const start: Point = {
82+
row: change.range.start.line,
83+
column: change.range.start.character,
84+
};
85+
const end: Point = {
86+
row: change.range.end.line,
87+
column: change.range.end.character,
88+
};
89+
const { edit, newContent } = createEdit(currentContent, change.text, start, end);
90+
components.syntaxTreeManager.updateWithEdit(documentUri, newContent, edit);
91+
currentContent = newContent;
92+
} else {
93+
// Full document change
94+
components.syntaxTreeManager.add(documentUri, change.text);
95+
currentContent = change.text;
96+
}
97+
}
98+
} catch (error) {
99+
log.error({ error, uri: documentUri, version }, 'Error updating tree - recreating');
100+
components.syntaxTreeManager.add(documentUri, finalContent);
113101
}
114-
});
102+
} else {
103+
// If we don't have a tree yet, just parse the final document
104+
components.syntaxTreeManager.add(documentUri, finalContent);
105+
}
115106

116-
// Trigger Guard validation
117-
components.guardService
118-
.validateDelayed(content, documentUri, ValidationTrigger.OnChange, true)
119-
.catch((reason) => {
120-
// Handle both getTextDocument and validation errors
121-
if (reason instanceof Error && reason.message.includes('Request cancelled')) {
122-
// Do nothing
123-
} else {
124-
log.error(reason, `Error in Guard didChange processing for ${documentUri}`);
125-
}
126-
});
107+
triggerValidation(
108+
components,
109+
finalContent,
110+
documentUri,
111+
LintTrigger.OnChange,
112+
ValidationTrigger.OnChange,
113+
true,
114+
);
127115

128116
// Republish validation diagnostics if available
129117
const validationDetails = components.validationManager
@@ -171,40 +159,33 @@ export function didSaveHandler(components: ServerComponents): (event: TextDocume
171159
const documentUri = event.document.uri;
172160
const documentContent = event.document.getText();
173161

174-
// Trigger cfn-lint validation
175-
components.cfnLintService.lintDelayed(documentContent, documentUri, LintTrigger.OnSave).catch((reason) => {
176-
if (reason instanceof CancellationError) {
177-
// Do nothing - cancellation is expected behavior
178-
} else {
179-
log.error(reason, `Linting error for ${documentUri}`);
180-
}
181-
});
182-
183-
// Trigger Guard validation
184-
components.guardService
185-
.validateDelayed(documentContent, documentUri, ValidationTrigger.OnSave)
186-
.catch((reason) => {
187-
if (reason instanceof Error && reason.message.includes('Request cancelled')) {
188-
// Do nothing
189-
} else {
190-
log.error(reason, `Guard validation error for ${documentUri}`);
191-
}
192-
});
162+
triggerValidation(components, documentContent, documentUri, LintTrigger.OnSave, ValidationTrigger.OnSave);
193163

194164
components.documentManager.sendDocumentMetadata(0);
195165
};
196166
}
197167

198-
function updateSyntaxTree(syntaxTreeManager: SyntaxTreeManager, textDocument: TextDocument, edit: Edit) {
199-
const uri = textDocument.uri;
200-
const document = new Document(textDocument);
201-
if (syntaxTreeManager.getSyntaxTree(uri)) {
202-
if (document.cfnFileType === CloudFormationFileType.Other) {
203-
syntaxTreeManager.deleteSyntaxTree(uri);
168+
function triggerValidation(
169+
components: ServerComponents,
170+
content: string,
171+
uri: string,
172+
lintTrigger: LintTrigger,
173+
validationTrigger: ValidationTrigger,
174+
debounce?: boolean,
175+
): void {
176+
components.cfnLintService.lintDelayed(content, uri, lintTrigger, debounce).catch((reason) => {
177+
if (reason instanceof CancellationError) {
178+
// Do nothing - cancellation is expected behavior
179+
} else {
180+
log.error(reason, `Linting error for ${uri}`);
181+
}
182+
});
183+
184+
components.guardService.validateDelayed(content, uri, validationTrigger, debounce).catch((reason) => {
185+
if (reason instanceof Error && reason.message.includes('Request cancelled')) {
186+
// Do nothing
204187
} else {
205-
syntaxTreeManager.updateWithEdit(uri, document.contents(), edit);
188+
log.error(reason, `Guard validation error for ${uri}`);
206189
}
207-
} else {
208-
syntaxTreeManager.addWithTypes(uri, document.contents(), document.documentType, document.cfnFileType);
209-
}
190+
});
210191
}

‎tst/e2e/DocumentHandler.test.ts‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,4 +99,107 @@ describe('DocumentHandler', () => {
9999
expect(extension.components.documentManager.get(uri)).toBeUndefined();
100100
}, 2_500);
101101
});
102+
103+
it('should create syntax tree for template documents on open', async () => {
104+
const content = 'AWSTemplateFormatVersion: "2010-09-09"\nResources: {}';
105+
106+
await extension.openDocument({
107+
textDocument: {
108+
uri,
109+
languageId: 'yaml',
110+
version: 1,
111+
text: content,
112+
},
113+
});
114+
115+
await WaitFor.waitFor(() => {
116+
const tree = extension.components.syntaxTreeManager.getSyntaxTree(uri);
117+
expect(tree).toBeDefined();
118+
expect(tree?.content()).toBe(content);
119+
});
120+
});
121+
122+
it('should update syntax tree on incremental document changes', async () => {
123+
const initialContent = 'AWSTemplateFormatVersion: "2010-09-09"\nResources: {}';
124+
125+
await extension.openDocument({
126+
textDocument: {
127+
uri,
128+
languageId: 'yaml',
129+
version: 1,
130+
text: initialContent,
131+
},
132+
});
133+
134+
await WaitFor.waitFor(() => {
135+
expect(extension.components.syntaxTreeManager.getSyntaxTree(uri)).toBeDefined();
136+
});
137+
138+
const editRange = { start: { line: 0, character: 35 }, end: { line: 0, character: 37 } };
139+
const editText = '00';
140+
const expectedContent = TestExtension.applyEdit(initialContent, editRange, editText);
141+
142+
await extension.changeDocument({
143+
textDocument: { uri, version: 2 },
144+
contentChanges: [
145+
{
146+
range: editRange,
147+
text: editText,
148+
},
149+
],
150+
});
151+
152+
await WaitFor.waitFor(() => {
153+
const tree = extension.components.syntaxTreeManager.getSyntaxTree(uri);
154+
expect(tree).toBeDefined();
155+
expect(tree?.content()).toBe(expectedContent);
156+
});
157+
});
158+
159+
it('should delete syntax tree when document is closed', async () => {
160+
const content = 'AWSTemplateFormatVersion: "2010-09-09"';
161+
162+
await extension.openDocument({
163+
textDocument: {
164+
uri,
165+
languageId: 'yaml',
166+
version: 1,
167+
text: content,
168+
},
169+
});
170+
171+
await WaitFor.waitFor(() => {
172+
expect(extension.components.syntaxTreeManager.getSyntaxTree(uri)).toBeDefined();
173+
});
174+
175+
await extension.closeDocument({
176+
textDocument: { uri },
177+
});
178+
179+
await WaitFor.waitFor(() => {
180+
expect(extension.components.syntaxTreeManager.getSyntaxTree(uri)).toBeUndefined();
181+
});
182+
});
183+
184+
it('should not create syntax tree for non-template documents', async () => {
185+
const content = 'someKey: someValue\nanotherKey: anotherValue';
186+
187+
await extension.openDocument({
188+
textDocument: {
189+
uri,
190+
languageId: 'yaml',
191+
version: 1,
192+
text: content,
193+
},
194+
});
195+
196+
await WaitFor.waitFor(() => {
197+
expect(extension.components.documentManager.get(uri)).toBeDefined();
198+
});
199+
200+
// Give it time to potentially create a tree (it shouldn't)
201+
await new Promise((resolve) => setTimeout(resolve, 100));
202+
203+
expect(extension.components.syntaxTreeManager.getSyntaxTree(uri)).toBeUndefined();
204+
});
102205
});

0 commit comments

Comments
 (0)