cleanup

Author: JohannesHoppe

.github/workflows/test.yml

@@ -17,13 +17,8 @@ jobs:
     steps:
       - name: Checkout code
         uses: actions/checkout@v4
-
-      - name: Checkout old-typescript-hero submodule at pinned commit
-        run: |
-          git submodule update --init --depth 1
-          cd comparison-test-harness/old-typescript-hero
-          git fetch --depth 1 origin 2cc666ec1d7d443aba1466a94e6e4c50a1424b83
-          git checkout 2cc666ec1d7d443aba1466a94e6e4c50a1424b83
+        with:
+          submodules: true  # Checkout submodule at pinned commit from repo
 
       - name: Setup Node.js
         uses: actions/setup-node@v4

CLAUDE.md

@@ -280,7 +280,7 @@ assert.equal(newResult, expected, 'New extension must produce correct output');
 
 ---
 
-## 📝 Configuration (13 Options)
+## 📝 Configuration (15 Options)
 
 All settings are under `miniTypescriptHero.imports.*`:
 
@@ -305,6 +305,10 @@ All settings are under `miniTypescriptHero.imports.*`:
 ### Blank Lines
 13. `blankLinesAfterImports` (one/two/preserve/legacy) - How many blank lines after imports
 
+### Behavior & Compatibility
+14. `organizeOnSave` (boolean) - Automatically organize imports when saving files
+15. `legacyMode` (boolean) - **INTERNAL!** Replicate old TypeScript Hero bugs exactly (auto-set by migration)
+
 ---
 
 ## 🐛 Bug Status (Session 18 Update)

comparison-test-harness/debug-parser.ts

@@ -491,4 +491,35 @@ const result = filter(doubled, x => x > 2).reduce((acc, val) => acc + val, 0);
     assert.equal(oldResult, expected, 'Old extension must produce correct output');
     assert.equal(newResult, expected, 'New extension must produce correct output');
   });
+
+  test('123. Comments between imports: Indentation preserved', async () => {
+    // PROOF: Both extensions preserve comment indentation when moving comments after imports
+    // OLD EXTENSION: Puts blank line AFTER imports, BEFORE comments, BEFORE code
+    // NEW EXTENSION: Matches old extension exactly (indentation preserved)
+    const input = `import { B } from './b';
+  // This is an indented comment
+import { A } from './a';
+    // This is a more indented comment
+import { C } from './c';
+
+console.log(A, B, C);
+`;
+
+    const oldResult = await organizeImportsOld(input);
+    const newResult = await organizeImportsNew(input);
+
+    // Expected: Imports sorted, comments moved after with indentation preserved
+    // VERIFIED from REAL old extension output (ran test to capture actual behavior)
+    const expected = `import { A } from './a';
+import { B } from './b';
+import { C } from './c';
+
+  // This is an indented comment
+    // This is a more indented comment
+console.log(A, B, C);
+`;
+
+    assert.equal(oldResult, expected, 'Old extension must produce correct output');
+    assert.equal(newResult, expected, 'New extension must produce correct output');
+  });
 });

comparison-test-harness/debug-test.js

@@ -253,6 +253,31 @@ export class ImportManager {
     }
   }
 
+  /**
+   * Helper method to remove trailing /index from import library names.
+   * Creates new Import objects instead of mutating readonly properties.
+   */
+  private removeTrailingIndexFromImports(imports: Import[]): Import[] {
+    return imports.map(imp => {
+      if (!imp.libraryName.endsWith('/index')) {
+        return imp;
+      }
+      const newLibraryName = imp.libraryName.replace(/\/index$/, '');
+
+      // Create new import object instead of mutating readonly property
+      if (imp instanceof NamedImport) {
+        return new NamedImport(newLibraryName, imp.specifiers, imp.defaultAlias);
+      } else if (imp instanceof NamespaceImport) {
+        return new NamespaceImport(newLibraryName, imp.alias);
+      } else if (imp instanceof ExternalModuleImport) {
+        return new ExternalModuleImport(newLibraryName, imp.alias);
+      } else if (imp instanceof StringImport) {
+        return new StringImport(newLibraryName);
+      }
+      return imp;
+    });
+  }
+
   /**
    * Organize imports: remove unused, sort, and group.
    * Returns TextEdits to apply the changes.
@@ -351,9 +376,7 @@ export class ImportManager {
     // In modern mode, /index removal happens first so imports like './lib/index' and './lib' can merge
     // In legacy mode, we do it after merging to replicate the old extension's bug
     if (this.config.removeTrailingIndex(this.document.uri) && !this.config.legacyMode(this.document.uri)) {
-      for (const imp of keep.filter(lib => lib.libraryName.endsWith('/index'))) {
-        imp.libraryName = imp.libraryName.replace(/\/index$/, '');
-      }
+      keep = this.removeTrailingIndexFromImports(keep);
     }
 
     //  Merge imports from same module (configurable)
@@ -448,9 +471,7 @@ export class ImportManager {
     // In legacy mode, we replicate the old extension's bug where /index removal happens after merging
     // This means './lib/index' and './lib' won't merge because they're different at merge time
     if (this.config.removeTrailingIndex(this.document.uri) && this.config.legacyMode(this.document.uri)) {
-      for (const imp of keep.filter(lib => lib.libraryName.endsWith('/index'))) {
-        imp.libraryName = imp.libraryName.replace(/\/index$/, '');
-      }
+      keep = this.removeTrailingIndexFromImports(keep);
     }
 
     // Group imports
@@ -517,10 +538,12 @@ export class ImportManager {
     // Extract comments between imports (old TypeScript Hero moves them after imports)
     const commentsBetweenImports: string[] = [];
     for (let i = importSectionStartLine; i <= importSectionEndLine; i++) {
-      const lineText = this.document.lineAt(i).text.trim();
+      const lineText = this.document.lineAt(i).text;
+      const trimmedText = lineText.trim();
       // Check if line is a comment (and not an import statement)
-      if ((lineText.startsWith('//') || lineText.startsWith('/*') || lineText.startsWith('*')) &&
-          !lineText.includes('import ')) {
+      // Check the trimmed version but preserve the original with indentation
+      if ((trimmedText.startsWith('//') || trimmedText.startsWith('/*') || trimmedText.startsWith('*')) &&
+          !trimmedText.includes('import ')) {
         commentsBetweenImports.push(lineText);
       }
     }

comparison-test-harness/get-actual-output.ts

@@ -2880,4 +2880,34 @@ const r = React;
       await deleteTempDocument(doc);
     }
   });
+
+  test('90. Comments between imports: Indentation preserved', async () => {
+    // CRITICAL: Comments between imports should preserve their indentation
+    // Scenario: Comments with 2-space indentation between imports
+    // Expected: Comments moved after imports WITH their original indentation
+
+    const content = `import { B } from './b';
+  // This is an indented comment
+import { A } from './a';
+
+console.log(A, B);
+`;
+
+    const doc = await createTempDocument(content);
+    try {
+      const manager = new ImportManager(doc, config);
+      const edits = manager.organizeImports();
+      const result = await applyEditsToDocument(doc, edits);
+
+      // Verify imports are organized
+      assert.ok(result.includes("import { A } from './a';"), 'Should have import A');
+      assert.ok(result.includes("import { B } from './b';"), 'Should have import B');
+
+      // CRITICAL: Verify comments preserve their indentation
+      assert.ok(result.includes('  // This is an indented comment'),
+        'Two-space indented comment should preserve indentation');
+    } finally {
+      await deleteTempDocument(doc);
+    }
+  });
 });