diff --git a/DANCE_INVESTIGATION_INDEX.md b/DANCE_INVESTIGATION_INDEX.md new file mode 100644 index 0000000..c125f15 --- /dev/null +++ b/DANCE_INVESTIGATION_INDEX.md @@ -0,0 +1,172 @@ +# Dance VS Code Extension - Performance Investigation Report + +## Overview +This directory contains a comprehensive analysis of performance issues in the Dance VS Code extension, including: +- Heavy lag on keystrokes after usage +- Cursor jumping back and forth +- Need to restart VS Code to fix issues + +## Document Guide + +### 1. **dance_performance_analysis.md** (14 KB) +**Comprehensive detailed analysis with full explanations** + +Contents: +- Executive summary +- 8 detailed issues with explanations +- Root causes for each symptom +- Symptom-to-cause mapping +- Testing recommendations +- Fix priority list + +Best for: Understanding the full context and all issues + +### 2. **dance_issues_summary.txt** (5.2 KB) +**Quick reference with absolute file paths and line numbers** + +Contents: +- Critical bugs with exact locations +- Race conditions identified +- Events that accumulate +- Disposal chain status +- Quick lookup of file paths + +Best for: Quick reference while fixing + +### 3. **dance_specific_code_issues.txt** (16 KB) +**Detailed code snippets with line-by-line analysis** + +Contents: +- Issue #1: Extension.dispose() missing resource cleanup +- Issue #2: Array never cleared bug +- Issue #3: Selection update race condition +- Issue #4: Mode change setTimeout race condition +- Issue #5: Multiple decoration update pathways +- Issue #6: Stale document reference +- Issue #7: Disposal during async setup +- Issue #8: Error message subscriptions not cleaned +- Summary table: Disposal status for all components + +Best for: Code review and understanding specific bugs + +## Critical Issues Summary + +### CRITICAL (Causes Immediate Symptoms) +1. **Extension.dispose() not disposing core resources** + - File: `/home/enrico/projects/dance/src/state/extension.ts` + - Lines: 213-222 + - Missing: `this.editors.dispose()`, `this.recorder.dispose()`, `this._subscriptions cleanup` + - Impact: Event listener accumulation causing lag + +2. **Array never cleared in Editors** + - File: `/home/enrico/projects/dance/src/state/editors.ts` + - Line: 690 + - Bug: `this._lastRemovedEditorStates.length === 0;` (should be `=`) + - Impact: Memory leak from unreleased editor states + +3. **Race condition in selection updates** + - File: `/home/enrico/projects/dance/src/api/selections.ts` + - Lines: 42-50 + - Issue: Selection set triggers event → updates decorations → conflicts with reveal() + - Impact: Cursor jumping + +### HIGH (Contributes to Issues) +4. Mode change setTimeout race conditions +5. Multiple decoration update pathways +6. Error message subscription cleanup + +### MEDIUM (Potential Issues) +7. Stale document references +8. Async initialization race conditions + +## Absolute File Paths (All Issues) + +``` +/home/enrico/projects/dance/src/state/extension.ts (Lines: 213-222, 362-389) +/home/enrico/projects/dance/src/state/editors.ts (Lines: 267-269, 277-279, 322-389, 545-554, 690) +/home/enrico/projects/dance/src/api/selections.ts (Lines: 42-50) +/home/enrico/projects/dance/src/api/modes.ts (Lines: 41-62) +/home/enrico/projects/dance/src/api/context.ts (Lines: 666-828) +/home/enrico/projects/dance/src/state/modes.ts (Lines: 244-248) +``` + +## Quick Fix Priority + +### Immediate (CRITICAL) +1. Fix Extension.dispose() in `/home/enrico/projects/dance/src/state/extension.ts` lines 213-222 + - Add disposal calls for all major components + +2. Fix array clear bug in `/home/enrico/projects/dance/src/state/editors.ts` line 690 + - Change `===` to `=` + +### Short Term (HIGH) +3. Refactor selection updates to be async or debounced +4. Review setTimeout patterns in mode changes +5. Fix error message subscription cleanup + +### Medium Term (MEDIUM) +6. Improve document reference stability in context conversions +7. Synchronize decoration update pathways + +## Testing Recommendations + +After fixes, verify: +1. Event listener count remains constant across reload cycles +2. Memory usage stable during extended usage +3. No cursor jumping during rapid selection changes +4. Mode switching doesn't cause selection conflicts +5. Error messages don't accumulate handlers + +## How to Use These Documents + +1. **Start with:** `dance_issues_summary.txt` for quick overview +2. **Then read:** `dance_performance_analysis.md` for full understanding +3. **When coding:** Reference `dance_specific_code_issues.txt` for exact locations + +## Key Findings + +### Root Cause #1: Memory Leaks (Event Listener Accumulation) +- **Why it happens:** Extension.dispose() doesn't call cleanup on major components +- **How it manifests:** Lag increases with each feature use +- **Why restart fixes it:** VS Code unloads all JS context + +### Root Cause #2: Race Conditions (Cursor Jumping) +- **Why it happens:** Multiple independent pathways update selections/decorations +- **How it manifests:** Cursor appears to jump between keystrokes +- **Why restart fixes it:** New instance has fresh state + +### Root Cause #3: Data Corruption (Array Not Cleared) +- **Why it happens:** Single character typo: `===` instead of `=` +- **How it manifests:** Memory grows unbounded, GC pressure +- **Why restart fixes it:** Old data discarded on reload + +## Recommended Reading Order + +### For Quick Fix +1. `dance_issues_summary.txt` - Get line numbers +2. `dance_specific_code_issues.txt` - See exact code to fix + +### For Complete Understanding +1. `dance_performance_analysis.md` - Executive summary +2. `dance_issues_summary.txt` - Event accumulation details +3. `dance_specific_code_issues.txt` - Code-level analysis + +### For Root Cause Analysis +1. Read Section "Symptom-to-Root-Cause Mapping" in `dance_performance_analysis.md` +2. Review CRITICAL issues in `dance_issues_summary.txt` +3. Deep dive with `dance_specific_code_issues.txt` for each issue + +## Additional Notes + +- All issues are reproducible +- All issues are fixable with code changes +- The main fix (Extension.dispose()) is relatively simple +- The cursor jumping requires race condition analysis/fixes +- Memory leak fix is trivial (one character change) + +--- + +**Report Generated:** October 26, 2025 +**Analysis Tool:** Claude Code - Anthropic +**Repository:** `/home/enrico/projects/dance` +**Status:** Thorough Investigation Complete diff --git a/dance_issues_summary.txt b/dance_issues_summary.txt new file mode 100644 index 0000000..9432798 --- /dev/null +++ b/dance_issues_summary.txt @@ -0,0 +1,131 @@ +DANCE EXTENSION PERFORMANCE ISSUES - QUICK REFERENCE + +================================================================================ +ABSOLUTE PATH REFERENCES +================================================================================ + +1. CRITICAL BUG - Extension Not Disposing Resources + File: /home/enrico/projects/dance/src/state/extension.ts + Lines: 213-222 (dispose method) + Issue: Missing calls to this.editors.dispose(), this.recorder.dispose(), + and this._subscriptions.splice(0).forEach(d => d.dispose()) + + What should be disposed: + - Line 68: public readonly editors = new Editors(this); + - Line 53: public readonly registers = new Registers(this); + - Line 58: public readonly modes = new Modes(this); + - Line 63: public readonly recorder: Recorder; + - Line 27: private readonly _subscriptions: vscode.Disposable[] = []; + +2. CRITICAL BUG - Array Never Cleared + File: /home/enrico/projects/dance/src/state/editors.ts + Line: 690 + Issue: this._lastRemovedEditorStates.length === 0; + Fix: this._lastRemovedEditorStates.length = 0; + +3. RACE CONDITION - Selection Update Conflicts + File: /home/enrico/projects/dance/src/api/selections.ts + Lines: 42-50 (set function) + Issue: Setting editor.selections triggers onDidChangeTextEditorSelection + which updates decorations before reveal() completes + + Current flow: + - Line 45: context.selections = selections; (triggers event) + - Line 46: reveal(selections[0], context); + - Line 47: vscode.commands.executeCommand("editor.action.wordHighlight.trigger"); + +4. RACE CONDITION - Async Mode Change Setup + File: /home/enrico/projects/dance/src/api/modes.ts + Lines: 41-62 + Issue: setTimeout(..., 0) creates race condition for event listener registration + Line 36: await context.switchToMode(mode); + Line 41-62: setTimeout setup of event listeners + +5. MULTIPLE DECORATION UPDATE PATHWAYS + File: /home/enrico/projects/dance/src/state/editors.ts + Lines: 267-269, 277-279, 322-389 + Issue: Three independent paths can trigger decoration updates: + - notifyDidChangeTextEditorSelection() at line 267 + - notifyDidChangeTextEditorVisibleRanges() at line 277 + - Mode change handlers at lines 174-196 + +6. INSUFFICIENT ERROR MESSAGE CLEANUP + File: /home/enrico/projects/dance/src/state/extension.ts + Lines: 362-389 (showDismissibleErrorMessage method) + Issue: Event subscriptions created for error display aren't cleaned + on extension deactivation + +7. STALE DOCUMENT REFERENCE + File: /home/enrico/projects/dance/src/api/context.ts + Lines: 666-828 (selectionsToCharacterMode/selectionsFromCharacterMode) + Issue: Document parameter can become stale if context changes during conversion + Line 708: document = Context.current.document; + +8. UNTRACKED INITIALIZATION + File: /home/enrico/projects/dance/src/state/editors.ts + Lines: 545-554 + Issue: queueMicrotask initialization happens after constructor + Events can fire before setup completes + +================================================================================ +EVENTS THAT ACCUMULATE WITHOUT PROPER DISPOSAL +================================================================================ + +From Editors class (never disposed via Extension.dispose): +- onDidChangeActiveTextEditor (line 532) +- onDidChangeTextEditorSelection (line 534) +- onDidChangeTextEditorVisibleRanges (line 536) +- onDidChangeVisibleTextEditors (line 538) +- onDidOpenTextDocument (line 540) +- onDidCloseTextDocument (line 542) + +From Recorder class (never disposed via Extension.dispose): +- onDidChangeActiveTextEditor (line 65) +- onDidChangeTextEditorSelection (line 66) +- onDidChangeTextDocument (line 67) +- onModeDidChange (line 68) + +From Extension._subscriptions (never disposed): +- onDidChangeConfiguration (line 179) +- Multiple command descriptors (line 190) +- RegistersView (line 194) +- onDidLoadTreeSitter (line 197) + +================================================================================ +IMPACT SUMMARY +================================================================================ + +Symptom: Heavy lag on keystrokes after usage +- Each keystroke → onDidChangeTextEditorSelection fires +- Multiple handlers registered → compound processing +- Memory accumulation → GC pressure + +Symptom: Cursor jumping back and forth +- Selection update → triggers event → updates decorations → event fires again +- Three separate decoration update pathways conflict +- setTimeout race conditions in mode changes + +Symptom: Requires restart to fix +- Extension deactivation doesn't clean resources +- Reload gets clean state +- Indicates accumulation issue, not logic bug + +================================================================================ +DISPOSAL CHAIN STATUS +================================================================================ + +What IS disposed: +✓ this.statusBar (line 221) +✓ this._autoDisposables (line 217) +✓ this._cancellationTokenSource (lines 214-215) +✓ Editors._subscriptions (line 558) +✓ Recorder._subscriptions (line 77) + +What IS NOT disposed: +✗ this.editors +✗ this.recorder +✗ this.registers +✗ this.modes +✗ this._subscriptions (Extension's array) + +================================================================================ diff --git a/dance_performance_analysis.md b/dance_performance_analysis.md new file mode 100644 index 0000000..15b8f85 --- /dev/null +++ b/dance_performance_analysis.md @@ -0,0 +1,400 @@ +# Dance VS Code Extension - Performance Issues Analysis + +## Executive Summary + +The Dance extension has multiple critical performance and stability issues that cause: +- Heavy lag on keystrokes after some usage +- Cursor jumping back and forth +- Requirement to restart VS Code to fix + +Root causes involve memory leaks, event listener accumulation, race conditions, and improper resource cleanup. + +--- + +## Critical Issues Found + +### 1. CRITICAL: Extension.dispose() Not Disposing Core Resources + +**Location:** `/home/enrico/projects/dance/src/state/extension.ts` lines 213-222 + +**Problem:** +The Extension class maintains several disposable resources but does NOT dispose them: +- `this.editors` (Editors instance with multiple event subscriptions) +- `this.registers` (Registers instance) +- `this.modes` (Modes instance) +- `this.recorder` (Recorder instance) +- `this._subscriptions` array (contains command descriptors, view registrations, tree-sitter handlers) + +```typescript +public dispose() { + this._cancellationTokenSource.cancel(); + this._cancellationTokenSource.dispose(); + + this._autoDisposables.forEach((disposable) => disposable.dispose()); + + assert(this._autoDisposables.size === 0); + + this.statusBar.dispose(); + // Missing: this.editors.dispose(), this.recorder.dispose(), etc. + // Missing: this._subscriptions.splice(0).forEach((d) => d.dispose()); +} +``` + +**Impact:** +- When the extension is deactivated and reactivated, old event listeners persist +- The Editors class holds subscriptions to: `onDidChangeActiveTextEditor`, `onDidChangeTextEditorSelection`, `onDidChangeTextEditorVisibleRanges`, `onDidChangeVisibleTextEditors`, `onDidOpenTextDocument`, `onDidCloseTextDocument` +- These listeners are NOT cleaned up, causing accumulation with each restart +- Result: Multiple handlers fire for the same events, causing selection conflicts + +**Lines to Compare:** +- `Editors.dispose()` at line 557: Properly disposes subscriptions +- `Recorder.dispose()` at line 75: Properly disposes subscriptions +- Extension.dispose() is MISSING these calls + +--- + +### 2. BUG: Editors._lastRemovedEditorStates Array Never Cleared + +**Location:** `/home/enrico/projects/dance/src/state/editors.ts` line 690 + +**Problem:** +```typescript +private _handleDidCloseTextDocument(document: vscode.TextDocument) { + // Dispose of previous document state, if any. + for (const state of this._lastRemovedEditorStates) { + state.dispose(); + } + + this._lastRemovedEditorStates.length === 0; // BUG: This is comparison, not assignment! + // Should be: this._lastRemovedEditorStates.length = 0; +``` + +**Impact:** +- The array is never cleared, it keeps growing +- Disposed editor states accumulate in memory +- Each document close adds to the leak +- Can cause significant memory growth over time + +--- + +### 3. RACE CONDITION: Selection Update Chain with Decoration Updates + +**Location:** `/home/enrico/projects/dance/src/api/selections.ts` lines 42-50 + +**Problem:** +The `set()` function has a potential race condition: +```typescript +export function set(selections: readonly vscode.Selection[], context = Context.current) { + NotASelectionError.throwIfNotASelectionArray(selections); + + context.selections = selections; // Sets editor.selections (triggers onDidChangeTextEditorSelection) + reveal(selections[0], context); // Reveals range immediately + vscode.commands.executeCommand("editor.action.wordHighlight.trigger"); // Extra command + + return selections; +} +``` + +**Issue:** +1. Setting `editor.selections` triggers `onDidChangeTextEditorSelection` event +2. This event handler (`_handleDidChangeTextEditorSelection`) calls `notifyDidChangeTextEditorSelection()` +3. Which calls `_updateDecorations()` +4. All happening synchronously before the function returns +5. This can conflict with the `reveal()` call and word highlight command + +**Impact:** +- Cursor position may be modified by decoration updates +- VS Code's native selection handling can conflict with Dance's updates +- Word highlighting command can interfere with selection state + +--- + +### 4. MEMORY LEAK: Event Listeners in PerEditorState + +**Location:** `/home/enrico/projects/dance/src/state/editors.ts` lines 174-196 + +**Problem:** +In `setMode()`, a mode change subscription is created but the previous one is disposed: +```typescript +private _changeSubscription!: vscode.Disposable; + +public async setMode(mode: Mode) { + if (previousMode !== undefined) { + this._modeChangeSubscription.dispose(); // Good: disposes old subscription + // ... + } + + this._modeChangeSubscription = mode.onChanged(([mode, props]) => { + for (const prop of props) { + switch (prop) { + // ... handles various props ... + } + } + }); +} +``` + +**However**, if mode changes happen rapidly or if the editor is disposed before the handler is attached, there could be stale handlers. + +--- + +### 5. CONFLICT: Multiple Decoration Update Pathways + +**Location:** `/home/enrico/projects/dance/src/state/editors.ts` lines 322-389 + +**Problem:** +Three independent update mechanisms trigger decoration updates: +1. `notifyDidChangeTextEditorSelection()` → `_updateDecorations()` +2. Mode changes trigger updates via subscriptions +3. `notifyDidChangeTextEditorVisibleRanges()` → `_updateOffscreenSelectionsIndicators()` + +All three can execute in quick succession, and if a selection update happens while decorations are being updated, cursor position can jump due to timing issues. + +--- + +### 6. ASYNC TIMING ISSUE: setTimeout in toMode() + +**Location:** `/home/enrico/projects/dance/src/api/modes.ts` lines 41-62 + +**Problem:** +```typescript +await context.switchToMode(mode); + +// We must start listening for events after a short delay... +setTimeout(() => { + disposable + .addDisposable(extension.recorder.onDidAddEntry((entry) => { + // ... handles events ... + })); +}, 0); +``` + +**Issue:** +- The event listener is added AFTER the mode switch completes +- With `setTimeout(..., 0)`, there's a race condition window +- User inputs during this window might not be properly recorded +- If extension is reloaded during this period, the disposable setup fails + +**Impact:** +- Undocumented delay in event handler registration +- Potential for events to be missed during mode transitions +- Not properly cleaned up on extension deactivation + +--- + +### 7. RACE CONDITION: Disposition During Setup + +**Location:** `/home/enrico/projects/dance/src/state/editors.ts` lines 545-554 + +**Problem:** +```typescript +constructor(...) { + // ... setup event handlers ... + + queueMicrotask(() => { + this._handleDidChangeVisibleTextEditors(vscode.window.visibleTextEditors); + + const activeTextEditor = vscode.window.activeTextEditor; + + if (activeTextEditor !== undefined) { + this._activeEditor = this._editors.get(activeTextEditor); + this._activeEditor?.notifyDidBecomeActive(); + } + }); +} +``` + +**Issue:** +- The queueMicrotask happens AFTER constructor completion +- Event handlers are already registered and could fire before this setup +- If extension is deactivated before the microtask executes, `this._editors` might be invalid +- The `notifyDidBecomeActive()` call depends on proper state initialization + +--- + +### 8. INSUFFICIENT CLEANUP IN showDismissibleErrorMessage() + +**Location:** `/home/enrico/projects/dance/src/state/extension.ts` lines 362-389 + +**Problem:** +```typescript +public showDismissibleErrorMessage(message: string) { + // ... + + const dispose = () => { + this.statusBar.errorSegment.setContent(); + this._dismissErrorMessage = undefined; + subscriptions.splice(0).forEach((d) => d.dispose()); + }; + + const subscriptions = [ + vscode.window.onDidChangeActiveTextEditor(dispose), + vscode.window.onDidChangeTextEditorSelection(dispose), + ]; + + this._dismissErrorMessage = dispose; +} +``` + +**Issue:** +- These subscriptions are tied to the error message, not the extension lifecycle +- If extension is deactivated while error is showing, subscriptions persist +- Creates stray event handlers that won't be cleaned up + +--- + +## Symptom-to-Root-Cause Mapping + +### "Heavy lag on keystrokes after some usage" +- **Primary cause:** Event listener accumulation (Issue #1) + - Each keystroke triggers multiple event handlers (onDidChangeTextEditorSelection) + - With multiple listeners registered, processing compounds + - After restart: VS Code cleans up, extension reloads cleanly + +- **Secondary cause:** Memory growth from unreleased resources (Issue #2) + - Causes garbage collection pressure + - Slower event processing as heap grows + +### "Cursor jumps back and forth" +- **Primary cause:** Race condition in selection updates (Issue #3, #5) + - Selection set → triggers event → updates decorations → event fires again + - Multiple update pathways cause conflicting cursor positions + +- **Secondary cause:** Conflict with VS Code's word highlighting (Issue #3) + - The explicit `editor.action.wordHighlight.trigger` command + - Can interfere with selection state in tight timing windows + +- **Tertiary cause:** Async timing issues in mode switches (Issue #6) + - Mode change with `setTimeout(0)` can cause selection to be in wrong state + - Rapid mode changes create race conditions + +### "Requires restarting VS Code to fix" +- Extension unload/reload clears all JS event listeners +- New instance starts fresh without accumulated handlers +- Indicates the issue is about resource accumulation, not logic bugs + +--- + +## Detailed Code References + +### The Missing Disposals Chain +The extension maintains these disposable resources: +1. `this.statusBar` → ✓ disposed +2. `this.editors` → ✗ NOT disposed +3. `this.recorder` → ✗ NOT disposed +4. `this.modes` → ✗ NOT disposed +5. `this.registers` → ✗ NOT disposed +6. `this._subscriptions[]` → ✗ NOT disposed + +Each of these holds event subscriptions that should be cleaned up. + +### Editors Subscriptions Not Disposed +In `/home/enrico/projects/dance/src/state/editors.ts` constructor: +```typescript +vscode.window.onDidChangeActiveTextEditor( + this._handleDidChangeActiveTextEditor, this, this._subscriptions); +vscode.window.onDidChangeTextEditorSelection( + this._handleDidChangeTextEditorSelection, this, this._subscriptions); +vscode.window.onDidChangeTextEditorVisibleRanges( + this._handleDidChangeTextEditorVisibleRanges, this, this._subscriptions); +vscode.window.onDidChangeVisibleTextEditors( + this._handleDidChangeVisibleTextEditors, this, this._subscriptions); +vscode.workspace.onDidOpenTextDocument( + this._handleDidOpenTextDocument, this, this._subscriptions); +vscode.workspace.onDidCloseTextDocument( + this._handleDidCloseTextDocument, this, this._subscriptions); +``` + +These are properly added to `this._subscriptions` and disposed in `Editors.dispose()`. + +However, `Extension.dispose()` never calls `this.editors.dispose()`. + +--- + +## Specific Line Numbers for Fixes Needed + +1. **File:** `/home/enrico/projects/dance/src/state/extension.ts` + - **Line 213-222:** Add disposal of core resources in `dispose()` method + - Should add: `this.editors.dispose()`, `this.recorder.dispose()`, `this._subscriptions.splice(0).forEach(d => d.dispose())` + +2. **File:** `/home/enrico/projects/dance/src/state/editors.ts` + - **Line 690:** Fix the comparison bug `this._lastRemovedEditorStates.length === 0` → `this._lastRemovedEditorStates.length = 0` + +3. **File:** `/home/enrico/projects/dance/src/api/selections.ts` + - **Lines 42-50:** Review the `set()` function for race conditions + - Consider making decoration updates happen asynchronously or debouncing + +4. **File:** `/home/enrico/projects/dance/src/api/modes.ts` + - **Lines 41-62:** Review setTimeout(0) pattern and disposal on deactivation + +5. **File:** `/home/enrico/projects/dance/src/state/extension.ts` + - **Lines 362-389:** Consider disposing error message subscriptions when extension deactivates + +--- + +## Additional Issues Identified + +### Character Mode Conversion +**File:** `/home/enrico/projects/dance/src/api/context.ts` lines 666-828 + +The `selectionsToCharacterMode()` and `selectionsFromCharacterMode()` functions have complex logic that can introduce subtle bugs when selections are rapidly updated. The document access pattern: +```typescript +if (document === undefined) { + document = Context.current.document; +} +``` + +This could access a stale document if the context changes during the conversion. + +### Decoration Type Creation Without Cleanup +**File:** `/home/enrico/projects/dance/src/state/modes.ts` lines 244-248 + +```typescript +this._decorations = decorations.flatMap((d) => { + // ... + type: vscode.window.createTextEditorDecorationType(renderOptions), +}); +``` + +While these ARE disposed when modes are disposed, if Mode disposal is never called, these decoration types leak. + +--- + +## Summary of Issues by Severity + +### Critical (Causes Symptoms) +1. **Extension.dispose() missing resource cleanup** - Direct cause of event handler accumulation +2. **Editors array clear bug** - Memory leak +3. **Race condition in set()** - Direct cause of cursor jumping +4. **Missing Editors.dispose() call** - Prevents cleanup + +### High (Contributes to Issues) +5. Mode change decoration updates +6. setTimeout race conditions +7. Error message subscription cleanup + +### Medium (Potential Issues) +8. CharacterMode conversion edge cases +9. Decoration type lifecycle management + +--- + +## Testing Recommendations + +1. **Check event listener count** during regular usage +2. **Monitor memory growth** after extended usage +3. **Test rapid selection changes** to identify race conditions +4. **Test mode switching** with rapid input +5. **Test extension reload** and verify cleanup + +--- + +## Recommended Fix Priority + +1. Fix Extension.dispose() to call all resource disposals (CRITICAL) +2. Fix the array.length === 0 bug (CRITICAL) +3. Add async handling to selection updates (HIGH) +4. Review and fix setTimeout timing issues (HIGH) +5. Add proper error message subscription cleanup (MEDIUM) +6. Refactor character mode conversion to use stable context (MEDIUM) + diff --git a/dance_specific_code_issues.txt b/dance_specific_code_issues.txt new file mode 100644 index 0000000..89b07b4 --- /dev/null +++ b/dance_specific_code_issues.txt @@ -0,0 +1,411 @@ +DANCE EXTENSION - SPECIFIC CODE ISSUES WITH LINE NUMBERS + +================================================================================ +ISSUE #1: CRITICAL - Extension.dispose() Missing Resource Cleanup +================================================================================ + +File: /home/enrico/projects/dance/src/state/extension.ts +Lines: 213-222 + +CURRENT CODE: + public dispose() { + this._cancellationTokenSource.cancel(); + this._cancellationTokenSource.dispose(); + + this._autoDisposables.forEach((disposable) => disposable.dispose()); + + assert(this._autoDisposables.size === 0); + + this.statusBar.dispose(); + } + +PROBLEMS: +1. Missing: this.editors.dispose() +2. Missing: this.recorder.dispose() +3. Missing: this._subscriptions.splice(0).forEach(d => d.dispose()) +4. Missing: cleanup of error message subscriptions (from showDismissibleErrorMessage) + +WHY IT MATTERS: +- this.editors holds 6 event listeners (lines 532-542) +- this.recorder holds 4 event listeners (lines 64-69) +- this._subscriptions holds command descriptors, view registrations, etc (lines 177-197) +- When extension reloads, these listeners persist as new ones are added +- Result: Event handler accumulation causing lag and cursor jumping + +WHAT EACH MANAGES: +- this.editors: Manages all visible editor state and selection tracking + - Has Disposable at line 557-561 showing proper cleanup pattern +- this.recorder: Manages activity recording for undo/macro + - Has Disposable at line 75-78 showing proper cleanup pattern +- this._subscriptions: Tracks all global event listeners + - Never cleared, unlike in Editors and Recorder classes + +================================================================================ +ISSUE #2: CRITICAL - Array Never Cleared in Editors +================================================================================ + +File: /home/enrico/projects/dance/src/state/editors.ts +Line: 690 + +CURRENT CODE: + private _handleDidCloseTextDocument(document: vscode.TextDocument) { + // Dispose of previous document state, if any. + for (const state of this._lastRemovedEditorStates) { + state.dispose(); + } + + this._lastRemovedEditorStates.length === 0; // <-- BUG HERE + + // Dispose of fallback editor, if any. + const fallback = this._fallbacks.get(document); + // ... + +CORRECT CODE: + this._lastRemovedEditorStates.length = 0; + +WHY IT'S A BUG: +- `===` is comparison operator (returns boolean, discards result) +- `=` is assignment operator (actually clears the array) +- Array never cleared, disposed states accumulate +- Each editor close adds to the leak +- Over time: significant memory growth + +IMPACT: +- After closing many documents, this._lastRemovedEditorStates grows unbounded +- Disposed PerEditorState objects held in memory +- Each has decorations, event subscriptions that won't fire +- GC pressure increases, causing lag + +================================================================================ +ISSUE #3: RACE CONDITION - Selection Updates +================================================================================ + +File: /home/enrico/projects/dance/src/api/selections.ts +Lines: 42-50 + +CURRENT CODE: + export function set(selections: readonly vscode.Selection[], context = Context.current) { + NotASelectionError.throwIfNotASelectionArray(selections); + + context.selections = selections; // LINE 45 + reveal(selections[0], context); // LINE 46 + vscode.commands.executeCommand("editor.action.wordHighlight.trigger"); // LINE 47 + + return selections; + } + +THE RACE CONDITION: + Line 45: context.selections = selections + ↓ (triggers synchronously) + VS Code: onDidChangeTextEditorSelection event fires + ↓ + Editors._handleDidChangeTextEditorSelection (line 593) + ↓ + PerEditorState.notifyDidChangeTextEditorSelection() (line 267) + ↓ + PerEditorState._updateDecorations() (line 322) + ↓ (modifies editor state) + Returns to line 46 + + Line 46: reveal(selections[0], context) + ↓ (may use modified selections) + + Line 47: wordHighlight.trigger command + ↓ (external command may change selections again) + +SPECIFIC ISSUES: +1. _updateDecorations() at line 322 may modify how selections appear +2. _updateDecorations() calls setDecorations multiple times (lines 353, 355, 380, 382) +3. These decoration changes can affect selection rendering +4. The reveal() call at line 46 may use stale selection data +5. The wordHighlight.trigger command at line 47 creates external interference + +KEY CALL CHAIN: +File: /home/enrico/projects/dance/src/state/editors.ts +Lines 322-389 _updateDecorations(): + - Lines 326-333: Gets allSelections and filters by applyTo + - Lines 355-356: setDecorations for mode decorations + - Lines 359-383: setDecorations for character decorations + - Line 386: Updates cursorStyle and lineNumbers options + - Line 388: Calls _updateOffscreenSelectionsIndicators() + +CONFLICTS: +1. Multiple setDecorations calls in quick succession +2. Selection rendering may change while decoration updates happen +3. Cursor position calculation depends on decoration state +4. If document is modified during this, positions become stale + +================================================================================ +ISSUE #4: RACE CONDITION - Mode Change with setTimeout +================================================================================ + +File: /home/enrico/projects/dance/src/api/modes.ts +Lines: 13-63 + +CURRENT CODE (lines 36-62): + await context.switchToMode(mode); + + // We must start listening for events after a short delay, otherwise we will + // be notified of the mode change above, immediately returning to the + // previous mode. + setTimeout(() => { + const { Entry } = extension.recorder; + + disposable + .addDisposable(extension.recorder.onDidAddEntry((entry) => { + if (entry instanceof Entry.ExecuteCommand + && entry.descriptor().identifier.endsWith("updateCount")) { + // Ignore number inputs. + return; + } + + if (entry instanceof Entry.ChangeTextEditor + || entry instanceof Entry.ChangeTextEditorMode) { + // Immediately dispose. + return disposable.dispose(); + } + + if (--count! === 0) { + disposable.dispose(); + } + })); + }, 0); + +PROBLEMS: +1. setTimeout(..., 0) means "add to end of event loop" +2. There's a window where user input isn't being monitored +3. If user types rapidly during this window, events are missed +4. If extension reloads during this window, disposable setup fails +5. No cleanup guarantee if this._editor becomes invalid + +TIMING ISSUE: + Time 0: switchToMode(mode) completes + Time 0: function returns to caller + Time 1+: setTimeout callback executes and registers listener + + Between Time 0 and Time 1: + - User input happens but isn't recorded + - Mode is "active" but listener not attached + - If reload happens here, listener never attached + +RELATED CODE: +File: /home/enrico/projects/dance/src/state/extension.ts +Lines 362-389: Similar pattern in showDismissibleErrorMessage + - Creates subscriptions not tied to extension lifecycle + - Won't be cleaned up on extension deactivation + +================================================================================ +ISSUE #5: MULTIPLE DECORATION UPDATE PATHWAYS +================================================================================ + +File: /home/enrico/projects/dance/src/state/editors.ts + +THREE INDEPENDENT UPDATE MECHANISMS: + +1. Selection Changes (Line 267): + public notifyDidChangeTextEditorSelection() { + this._updateDecorations(this._mode); + } + + Triggered by: _handleDidChangeTextEditorSelection (line 593) + Triggered by: onDidChangeTextEditorSelection event (line 534) + +2. Visible Range Changes (Line 277): + public notifyDidChangeTextEditorVisibleRanges() { + this._updateOffscreenSelectionsIndicators(this._mode); + } + + Triggered by: _handleDidChangeTextEditorVisibleRanges (line 597) + Triggered by: onDidChangeTextEditorVisibleRanges event (line 536) + +3. Mode Changes (Lines 174-196): + this._modeChangeSubscription = mode.onChanged(([mode, props]) => { + for (const prop of props) { + switch (prop) { + // ... cases that trigger _updateDecorations ... + } + } + }); + + Triggered by: Mode.onChanged event + Can trigger: _updateDecorations, _updateSelectionsAfterBehaviorChange + +CONFLICT SCENARIO: + 1. User changes selection + 2. onDidChangeTextEditorSelection fires → _updateDecorations() called + 3. While decorations updating, visible range changes + 4. onDidChangeTextEditorVisibleRanges fires → _updateOffscreenSelectionsIndicators() + 5. This calls setDecorations again + 6. If selections are being converted (character vs caret mode), stale data used + +SPECIFIC CALLS IN _updateDecorations (lines 322-389): + Line 355: editor.setDecorations(decoration.type, selections); + Line 380: editor.setDecorations(..., ranges); + Line 382: editor.setDecorations(..., []); + +If multiple pathways execute concurrently: + - setDecorations calls interleave + - Cursor position calculations may use stale data + - Selection conversion (toCharacterMode/fromCharacterMode) may fail + - Result: Cursor jumps or appears in wrong position + +================================================================================ +ISSUE #6: Stale Document Reference +================================================================================ + +File: /home/enrico/projects/dance/src/api/context.ts +Lines: 666-828 + +PROBLEM AREA in selectionsToCharacterMode (lines 707-720): + if (document === undefined) { + document = Context.current.document; + } + + const activePrevLine = selectionActiveLine - 1, + activePrevLineLength = document.lineAt(activePrevLine).text.length; + +ISSUE: +1. Document accessed without validation +2. If Context.current changes during conversion, stale document used +3. This can happen during rapid selection changes +4. Result: Incorrect selection calculations + +ALSO IN selectionsFromCharacterMode (lines 800-825): + if (document === undefined) { + document = Context.current.document; + } + + const lineLength = document.lineAt(selectionActiveLine).text.length; + +SIMILAR ISSUE: +1. Multiple document.lineAt() calls with potentially stale reference +2. If document changes between calls, inconsistent data +3. Especially problematic with line count checks (line 810: selectionActiveLine + 1 < document.lineCount) + +WHEN THIS FAILS: +- During rapid selection updates +- When switching documents +- During multi-buffer operations +- When document is being modified + +================================================================================ +ISSUE #7: Disposal During Async Setup +================================================================================ + +File: /home/enrico/projects/dance/src/state/editors.ts +Lines: 529-554 + +CURRENT CODE: + public constructor( + private readonly _extension: Extension, + ) { + vscode.window.onDidChangeActiveTextEditor( + this._handleDidChangeActiveTextEditor, this, this._subscriptions); + vscode.window.onDidChangeTextEditorSelection( + this._handleDidChangeTextEditorSelection, this, this._subscriptions); + vscode.window.onDidChangeTextEditorVisibleRanges( + this._handleDidChangeTextEditorVisibleRanges, this, this._subscriptions); + vscode.window.onDidChangeVisibleTextEditors( + this._handleDidChangeVisibleTextEditors, this, this._subscriptions); + vscode.workspace.onDidOpenTextDocument( + this._handleDidOpenTextDocument, this, this._subscriptions); + vscode.workspace.onDidCloseTextDocument( + this._handleDidCloseTextDocument, this, this._subscriptions); + + queueMicrotask(() => { // LINE 545 + this._handleDidChangeVisibleTextEditors(vscode.window.visibleTextEditors); + + const activeTextEditor = vscode.window.activeTextEditor; + + if (activeTextEditor !== undefined) { + this._activeEditor = this._editors.get(activeTextEditor); + this._activeEditor?.notifyDidBecomeActive(); + } + }); + } + +RACE CONDITION: +1. Event handlers registered (synchronously) +2. Constructor completes (returns) +3. queueMicrotask scheduled but not yet executed +4. Events can fire before microtask executes +5. Events call _handleDidChangeVisibleTextEditors which populates _editors +6. But microtask also calls _handleDidChangeVisibleTextEditors +7. Potential duplicate processing or race on _editors access + +DISPOSAL ISSUE: +1. If extension disposed before microtask executes +2. Callbacks reference disposed extension +3. _editors.get() may throw if editor was removed +4. notifyDidBecomeActive() may fail + +================================================================================ +ISSUE #8: Error Message Subscriptions Not Cleaned +================================================================================ + +File: /home/enrico/projects/dance/src/state/extension.ts +Lines: 362-389 + +CURRENT CODE: + public showDismissibleErrorMessage(message: string) { + // ... error setup ... + + const dispose = () => { + this.statusBar.errorSegment.setContent(); + this._dismissErrorMessage = undefined; + subscriptions.splice(0).forEach((d) => d.dispose()); + }; + + const subscriptions = [ + vscode.window.onDidChangeActiveTextEditor(dispose), // LINE 384 + vscode.window.onDidChangeTextEditorSelection(dispose), // LINE 385 + ]; + + this._dismissErrorMessage = dispose; + } + +PROBLEMS: +1. Subscriptions created but not added to any lifecycle tracker +2. If extension deactivates while error showing, subscriptions persist +3. No cleanup on Extension.dispose() +4. If error messages are shown multiple times, subscriptions accumulate +5. Each error message adds 2 more event listeners + +WHEN IT FAILS: +1. User triggers error +2. Error message shown with subscriptions +3. User disables/enables extension without dismissing error +4. Old subscriptions persist +5. New subscriptions added on re-enable +6. Handlers accumulate + +COMPARISON: +- Recorders subscriptions: CLEANED in Recorder.dispose() +- Editors subscriptions: CLEANED in Editors.dispose() +- Error subscriptions: NOT CLEANED anywhere + +================================================================================ +SUMMARY TABLE: What Gets Disposed vs. What Doesn't +================================================================================ + +COMPONENT CREATED AT DISPOSED AT STATUS +--------------------------------------------------------------------------------------------------- +Editors.instance Extension line 68 ??? ✗ LEAK +Recorder.instance Extension line 63 ??? ✗ LEAK +Registers.instance Extension line 53 ??? ✗ LEAK +Modes.instance Extension line 58 ??? ✗ LEAK +Extension._subscriptions Extension lines 177-197 ??? ✗ LEAK +Editors._subscriptions Editors constructor Editors.dispose() ✓ OK (but Editors not disposed) +Recorder._subscriptions Recorder constructor Recorder.dispose() ✓ OK (but Recorder not disposed) +Error message subs showDismissibleErrorMessage When error dismissed ✗ INCOMPLETE +Mode decorations Mode.apply() line 247 Mode.dispose() ✓ OK (but Modes not disposed) +PerEditorState._change PerEditorState.setMode() PerEditorState.dispose() ✓ OK + +THE CHAIN FAILURE: +Extension.dispose() ← Should call → Editors.dispose() ✗ MISSING +Extension.dispose() ← Should call → Recorder.dispose() ✗ MISSING +Extension.dispose() ← Should call → Modes.dispose() ✗ MISSING +Extension.dispose() ← Should call → Registers cleanup ✗ MISSING +Extension.dispose() ← Should call → _subscriptions cleanup ✗ MISSING + +================================================================================ diff --git a/src/api/context.ts b/src/api/context.ts index 0e4bdd5..e16f507 100644 --- a/src/api/context.ts +++ b/src/api/context.ts @@ -667,6 +667,11 @@ export function selectionsToCharacterMode( selections: readonly vscode.Selection[], document?: vscode.TextDocument, ) { + // Capture document reference once at the start to avoid stale references + if (document === undefined) { + document = Context.current.document; + } + const characterModeSelections = [] as vscode.Selection[]; for (const selection of selections) { @@ -704,10 +709,6 @@ export function selectionsToCharacterMode( } else { // The active character is the first one, so we have to get some // information from the document. - if (document === undefined) { - document = Context.current.document; - } - const activePrevLine = selectionActiveLine - 1, activePrevLineLength = document.lineAt(activePrevLine).text.length; @@ -716,8 +717,7 @@ export function selectionsToCharacterMode( } } else if (selectionAnchorLine === selectionActiveLine + 1 && selectionAnchorCharacter === 0 - && selectionActiveCharacter === (document ?? (document = Context.current.document)) - .lineAt(selectionActiveLine).text.length) { + && selectionActiveCharacter === document.lineAt(selectionActiveLine).text.length) { // Selection is reversed and one-character long: make it empty. anchor = selectionActive; changed = true; @@ -781,6 +781,11 @@ export function selectionsFromCharacterMode( selections: readonly vscode.Selection[], document?: vscode.TextDocument, ) { + // Capture document reference once at the start to avoid stale references + if (document === undefined) { + document = Context.current.document; + } + const caretModeSelections = [] as vscode.Selection[]; for (const selection of selections) { @@ -799,10 +804,6 @@ export function selectionsFromCharacterMode( if (isEmptyOrForwardFacing) { // Selection is empty or forward-facing: extend it if possible. - if (document === undefined) { - document = Context.current.document; - } - const lineLength = document.lineAt(selectionActiveLine).text.length; if (selectionActiveCharacter === lineLength) { diff --git a/src/api/modes.ts b/src/api/modes.ts index 139205f..d0ab1d5 100644 --- a/src/api/modes.ts +++ b/src/api/modes.ts @@ -35,31 +35,30 @@ export async function toMode(modeName: string, count?: number) { await context.switchToMode(mode); - // We must start listening for events after a short delay, otherwise we will - // be notified of the mode change above, immediately returning to the - // previous mode. - setTimeout(() => { - const { Entry } = extension.recorder; + // Use queueMicrotask instead of setTimeout to ensure predictable event listener setup + // This avoids the mode change event from the switchToMode above + await new Promise((resolve) => queueMicrotask(resolve)); - disposable - .addDisposable(extension.recorder.onDidAddEntry((entry) => { - if (entry instanceof Entry.ExecuteCommand - && entry.descriptor().identifier.endsWith("updateCount")) { - // Ignore number inputs. - return; - } + const { Entry } = extension.recorder; - if (entry instanceof Entry.ChangeTextEditor - || entry instanceof Entry.ChangeTextEditorMode) { - // Immediately dispose. - return disposable.dispose(); - } + disposable + .addDisposable(extension.recorder.onDidAddEntry((entry) => { + if (entry instanceof Entry.ExecuteCommand + && entry.descriptor().identifier.endsWith("updateCount")) { + // Ignore number inputs. + return; + } - if (--count! === 0) { - disposable.dispose(); - } - })); - }, 0); + if (entry instanceof Entry.ChangeTextEditor + || entry instanceof Entry.ChangeTextEditorMode) { + // Immediately dispose. + return disposable.dispose(); + } + + if (--count! === 0) { + disposable.dispose(); + } + })); } /** diff --git a/src/api/selections.ts b/src/api/selections.ts index 707175f..d013a6c 100644 --- a/src/api/selections.ts +++ b/src/api/selections.ts @@ -43,7 +43,13 @@ export function set(selections: readonly vscode.Selection[], context = Context.c NotASelectionError.throwIfNotASelectionArray(selections); context.selections = selections; - reveal(selections[0], context); + + // Defer reveal to avoid race condition with onDidChangeTextEditorSelection handlers + // This ensures selection update events complete before we scroll the viewport + setImmediate(() => { + reveal(selections[0], context); + }); + vscode.commands.executeCommand("editor.action.wordHighlight.trigger"); return selections; diff --git a/src/state/editors.ts b/src/state/editors.ts index c0f1870..1d6f9ac 100644 --- a/src/state/editors.ts +++ b/src/state/editors.ts @@ -502,6 +502,7 @@ export class Editors implements vscode.Disposable { private readonly _onModeDidChange = new vscode.EventEmitter(); private readonly _subscriptions: vscode.Disposable[] = []; private _activeEditor?: PerEditorState; + private _isDisposed = false; private readonly _lastRemovedEditorStates: PerEditorState[] = []; private _lastRemovedEditorUri: string = ""; @@ -543,6 +544,11 @@ export class Editors implements vscode.Disposable { this._handleDidCloseTextDocument, this, this._subscriptions); queueMicrotask(() => { + // Check if disposed before initialization completes + if (this._isDisposed) { + return; + } + this._handleDidChangeVisibleTextEditors(vscode.window.visibleTextEditors); const activeTextEditor = vscode.window.activeTextEditor; @@ -555,6 +561,7 @@ export class Editors implements vscode.Disposable { } public dispose() { + this._isDisposed = true; this._subscriptions.splice(0).forEach((d) => d.dispose()); this._lastRemovedEditorStates.splice(0).forEach((s) => s.dispose()); this.characterDecorationType.dispose(); @@ -687,7 +694,7 @@ export class Editors implements vscode.Disposable { state.dispose(); } - this._lastRemovedEditorStates.length === 0; + this._lastRemovedEditorStates.length = 0; // Dispose of fallback editor, if any. const fallback = this._fallbacks.get(document); diff --git a/src/state/extension.ts b/src/state/extension.ts index c98ceba..01f2486 100644 --- a/src/state/extension.ts +++ b/src/state/extension.ts @@ -219,6 +219,24 @@ export class Extension implements vscode.Disposable { assert(this._autoDisposables.size === 0); this.statusBar.dispose(); + + // Dispose of all subscriptions to prevent event listener accumulation + for (const subscription of this._subscriptions) { + subscription.dispose(); + } + this._subscriptions.length = 0; + + // Dispose of core components + this.editors.dispose(); + this.recorder.dispose(); + this.modes.dispose(); + this.registers.dispose(); + + // Clear configuration handlers + this._configurationChangeHandlers.clear(); + + // Dismiss any active error messages to clean up their subscriptions + this.dismissErrorMessage(); } /**