Skip to content

Conversation

@atennak1
Copy link
Contributor

@atennak1 atennak1 commented Nov 19, 2025

Bug: Syntax Tree Corruption with Incremental Edits

Problem

When users typed quickly or made multiple edits in a single change event, the syntax tree became corrupted with ERROR nodes and nodes containing ": null" text. This caused incorrect follow-up (after the first) code action edits and application.

Root Cause

The didChangeHandler was using the final document content to calculate edits for each incremental change. Since each change's range was calculated
against intermediate content states, applying them with the final content caused incorrect edit positions and tree corruption. This was seemingly not a problem for manual code edits as the handler was probably only receiving one incremental update at a time, but a code action with multiple edits triggers this bug.

Buggy code flow:

  1. Extract to Parameter code action applies mutiple edits around the same time. In my case, 8 changes arrive to the DocumentHandler.
  2. Handler gets content = textDocument.getText() (final state)
  3. For each change, calculates edit using createEdit(content, change.text, start, end)
  4. All edits use the same content, but ranges assume intermediate states
  5. Tree becomes corrupted with ERROR nodes and corrupted strings

Solution

Track the current content state and apply each change sequentially:

  1. Start with current tree content (or empty if no tree)
  2. For each change, calculate edit using current content state
  3. Apply edit to tree with the new content after that specific change
  4. Update current content for next iteration

This ensures each incremental edit is applied correctly with proper content state, maintaining tree integrity while preserving performance through tree-sitter's incremental parsing.

Changes

src/handlers/DocumentHandler.ts: Fixed to apply edits sequentially with correct content state
src/context/syntaxtree/SyntaxTree.ts: Added getRootNode() public method for testing
tst/unit/handlers/DocumentHandler.test.ts: Added test reproducing exact edit sequence from production logs, validates no tree corruption
tst/resources/templates/sample_template_after_edits.json: Expected template state for test validation

Verification

  • all existing unit tests pass
  • multiple code action applications now valid
  • autocomplete, hover, etc. continue to work

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@atennak1 atennak1 requested a review from a team as a code owner November 19, 2025 19:52
kddejong
kddejong previously approved these changes Nov 19, 2025
@satyakigh
Copy link
Collaborator

I think lets hold off on merging this until we have all the bug fixes resolved from the release? This might require some in depth testing

const tree = components.syntaxTreeManager.getSyntaxTree(documentUri);

// Short-circuit if this is not a template (anymore)
if (document.cfnFileType === CloudFormationFileType.Other) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be cfnFileType !== CloudFormationFileType.Template ? we have gitsync, unknown, other and empty types now, don't think we need to do anything with gitsync files either

Copy link
Contributor Author

@atennak1 atennak1 Nov 26, 2025

Choose a reason for hiding this comment

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

Our current detection isn't perfect and we have lots of Completion tests that expect valid completions on templates even when cfnFileType !== CloudFormationFileType.Template

Zee2413
Zee2413 previously approved these changes Nov 26, 2025
satyakigh
satyakigh previously approved these changes Nov 26, 2025
@atennak1 atennak1 dismissed stale reviews from satyakigh and Zee2413 via 5cf052d November 26, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants