feat: Implement re-export preservation and fix all skipped tests

Implements complete re-export preservation in ImportManager, matching
old TypeScript Hero behavior. All skipped tests are now fixed and
passing, with zero known limitations remaining.

Key changes:
- Added reExports field to ImportManager to track re-export statements
- Modified extractImports() to capture both regular re-exports
  (export { X } from './m') and namespace re-exports
  (export * as utils from './utils')
- Updated generateTextEdits() to include export declarations in
  replacement range and append re-exports after organized imports
- Fixed A1 test to actively catch old extension crash instead of skip
- Fixed A7a/A7b tests with correct re-export preservation expectations
- Fixed B19 test to expect re-exports after imports (not in-place)
- Cleaned up redundant 'both extensions' wording from test descriptions

Re-export behavior:
- Re-exports are moved AFTER all imports (matching old extension)
- Re-exports from same module as imports are NOT merged
- Both export { X } from and export * as ns from are preserved
- Original order of re-exports is maintained

Test results:
- Main extension tests: 226 passing ✅
- Comparison harness tests: 147 passing ✅
- Total: 373 tests passing with ZERO failures or skipped tests
- All 'known limitations' resolved

This completes the parity implementation with old TypeScript Hero,
ensuring the new extension handles all edge cases correctly including
scenarios where the old extension crashes.

Author: JohannesHoppe

CLAUDE_TODO.md

@@ -4124,3 +4124,101 @@ Total:                    370 tests, 367 passing, 0 warnings
 
 **Thanks to user for insisting on finding the root cause instead of hiding the problem!**
 
+
+---
+
+## Session: 2025-10-27 - Re-export Preservation Implementation & Test Cleanup
+
+### Completed Tasks ✅
+
+1. **Implemented Re-export Preservation (A7a & A7b)**
+   - Added `reExports: string[]` field to ImportManager class to track re-export statements
+   - Modified `extractImports()` to capture both regular re-exports (`export { X } from './m'`) and namespace re-exports (`export * as utils from './utils'`)
+   - Updated `generateTextEdits()` range calculation to include export declarations in replacement range
+   - Appended re-exports after imports in output generation (matching old TypeScript Hero behavior)
+   - Both A7a and A7b comparison tests now passing
+
+2. **Fixed A1 Test - Old Extension Crash Handling**
+   - Changed from `.skip` to active test that catches the old extension's crash
+   - Old extension crashes with `TypeError: libraryAlreadyImported.specifiers is not iterable` when mixing side-effect and named imports from same module
+   - Test now validates the crash occurs AND that new extension handles it correctly
+   - New extension keeps side-effect and named imports as separate statements
+
+3. **Fixed B19 Test - Incorrect Expected Output**
+   - Test had wrong expectations (expected re-exports to stay in place)
+   - Updated to match actual old TypeScript Hero behavior: re-exports moved AFTER imports
+   - Now consistent with comparison tests A7a/A7b
+
+4. **Cleaned Up Test Wording**
+   - Removed redundant "both extensions" phrasing from test names and comments
+   - Updated A7a, A7b, A9 tests for cleaner descriptions
+   - Comparison tests inherently prove both extensions work - no need to state explicitly
+
+### Files Modified
+
+1. **`src/imports/import-manager.ts`**
+   - Added `private reExports: string[] = []` field (line 23)
+   - Added re-export extraction in `extractImports()` (lines 127-138)
+   - Updated `generateTextEdits()` to include export declarations in range (lines 550-554)
+   - Appended re-exports after imports in output generation (lines 687-691)
+
+2. **`comparison-test-harness/test-cases/10-additional-parity.test.ts`**
+   - Fixed A1 test: Changed from `.skip` to active test with try/catch for crash validation
+   - Updated A7a test: Removed "both extensions preserve them" wording, updated to expect re-exports after imports
+   - Updated A7b test: Removed "both extensions preserve them" wording, updated to expect namespace re-exports after imports
+   - Updated A9 test: Removed "both extensions" from title
+
+3. **`src/test/import-manager.edge-cases.test.ts`**
+   - Fixed B19 test expected output: Re-exports now correctly expected AFTER imports (not before)
+   - Updated assertion message to clarify behavior
+
+### Test Results
+
+- **Main extension tests**: 226 passing ✅
+- **Comparison harness tests**: 147 passing ✅
+- **Total**: 373 tests passing
+- **Skipped tests**: 0 (all fixed!)
+- **Known limitations**: 0 (all resolved!)
+
+### Technical Decisions
+
+1. **Re-export Placement**: Re-exports are moved AFTER all imports (matching old TypeScript Hero behavior)
+   - This applies to both `export { X } from './m'` and `export * as ns from './m'`
+   - Re-exports from the same module as imports are NOT merged with imports
+
+2. **Range Calculation**: Export declarations with module specifiers are included in the import section range
+   - Ensures re-exports mixed with imports are properly removed from original position
+   - Re-exports are then re-added after the organized imports
+
+3. **Test Pattern Consistency**: All comparison tests follow the mandatory pattern of validating against explicit expected output (never just comparing two results without validation)
+
+### Key Learnings
+
+1. **User Feedback Pattern**: User consistently rejects shortcuts/workarounds and demands proper fixes:
+   - "never hide errors like this!" (rejecting warning suppression)
+   - "know limitations? we have no known limitations!"
+   - "we fix everything and make the extension perfect!"
+
+2. **Re-export Behavior**: Old TypeScript Hero moves re-exports AFTER imports, not in-place
+   - Initially unclear from test artifact naming
+   - Confirmed by running actual old extension in comparison tests
+
+3. **Test Wording**: Saying "both extensions" in comparison tests is redundant
+   - ALL comparison tests prove both extensions work correctly
+   - More concise wording is better
+
+### Next Steps
+
+✅ All planned work completed for this session!
+
+**Future Considerations**:
+- Monitor for any edge cases with re-exports in real-world usage
+- Consider whether re-exports should be sorted among themselves (currently preserved in order found)
+
+### Session Notes
+
+- Started from previous session about listener leak warnings (already fixed)
+- User requested fixing all skipped tests and "known limitations"
+- Successfully implemented re-export preservation matching old extension behavior
+- Zero test failures, zero skipped tests, zero known limitations remaining
+

comparison-test-harness/test-cases/10-additional-parity.test.ts

@@ -17,31 +17,38 @@ suite('Additional Parity Tests (Second Audit)', () => {
   // A1: Side-effect + named from same module
   // ============================================================================
 
-  test.skip('A1: Side-effect + named from same module - OLD EXTENSION CRASHES', async () => {
+  test('A1: Side-effect + named from same module - OLD EXTENSION CRASHES, NEW HANDLES IT', async () => {
     const input = `import 'zone.js';
 import { a } from 'zone.js';
 
 const x = a;
 `;
 
-    // PROOF: Old extension CRASHES with:
+    // OLD extension CRASHES with:
     // TypeError: libraryAlreadyImported.specifiers is not iterable
     // at ImportManager.organizeImports (old-typescript-hero/src/imports/import-manager.js:134:59)
     //
     // This happens when it tries to merge a side-effect import with a named import
-    // from the same module.
-    //
-    // NEW extension handles this correctly - keeps them as separate imports with
-    // blank line separator (Plains group vs Workspace group).
-
+    // from the same module - it's a BUG in the old extension.
+
+    // Test old extension - it SHOULD crash
+    let oldCrashed = false;
+    try {
+      await organizeImportsOld(input, { legacyMode: true });
+    } catch (error: any) {
+      oldCrashed = true;
+      assert.ok(error.message.includes('is not iterable'), 'Old extension crashes with expected error');
+    }
+    assert.ok(oldCrashed, 'Old extension MUST crash on this input (known bug)');
+
+    // NEW extension handles this correctly - keeps them as separate imports
     const expectedNew = `import 'zone.js';
 
 import { a } from 'zone.js';
 
 const x = a;
 `;
 
-    // Only test new extension (old crashes - VERIFIED by running test without .skip)
     const newResult = await organizeImportsNew(input, { legacyMode: true });
     assert.strictEqual(newResult, expectedNew, 'New extension handles side-effect + named imports correctly');
   });
@@ -285,7 +292,7 @@ const z = M;
   // A7: Re-exports parity
   // ============================================================================
 
-  test.skip('A7a: Re-exports with export { X } from - NEW EXTENSION LIMITATION', async () => {
+  test('A7a: Re-exports with export { X } from', async () => {
     const input = `import { A } from './a';
 export { X } from './m';
 import { B } from './b';
@@ -294,33 +301,23 @@ const foo = A;
 const bar = B;
 `;
 
-    // OLD extension: Moves re-exports AFTER all imports
-    const expectedOld = `import { A } from './a';
+    // Expected: Re-exports are moved AFTER all imports
+    const expected = `import { A } from './a';
 import { B } from './b';
 
 export { X } from './m';
-const foo = A;
-const bar = B;
-`;
-
-    // NEW extension: Currently REMOVES re-exports (BUG - needs fixing)
-    // This is a limitation of ts-morph-based approach
-    // TODO: Fix this in future version
-    const expectedNew = `import { A } from './a';
-import { B } from './b';
-
 const foo = A;
 const bar = B;
 `;
 
     const oldResult = await organizeImportsOld(input, { legacyMode: true });
     const newResult = await organizeImportsNew(input, { legacyMode: true });
 
-    assert.strictEqual(oldResult, expectedOld, 'Old extension moves re-exports after imports');
-    assert.strictEqual(newResult, expectedNew, 'New extension removes re-exports (LIMITATION)');
+    assert.strictEqual(oldResult, expected, 'Old extension preserves re-exports after imports');
+    assert.strictEqual(newResult, expected, 'New extension preserves re-exports after imports');
   });
 
-  test.skip('A7b: Re-exports with export * as ns from - NEW EXTENSION LIMITATION', async () => {
+  test('A7b: Re-exports with export * as ns from', async () => {
     const input = `import { A } from './a';
 export * as utils from './utils';
 import { B } from './b';
@@ -329,29 +326,20 @@ const foo = A;
 const bar = B;
 `;
 
-    // OLD extension: Moves re-exports AFTER all imports
-    const expectedOld = `import { A } from './a';
+    // Expected: Namespace re-exports are moved AFTER all imports
+    const expected = `import { A } from './a';
 import { B } from './b';
 
 export * as utils from './utils';
-const foo = A;
-const bar = B;
-`;
-
-    // NEW extension: Currently REMOVES namespace re-exports (BUG - needs fixing)
-    // TODO: Fix this in future version
-    const expectedNew = `import { A } from './a';
-import { B } from './b';
-
 const foo = A;
 const bar = B;
 `;
 
     const oldResult = await organizeImportsOld(input, { legacyMode: true });
     const newResult = await organizeImportsNew(input, { legacyMode: true });
 
-    assert.strictEqual(oldResult, expectedOld, 'Old extension moves namespace re-exports after imports');
-    assert.strictEqual(newResult, expectedNew, 'New extension removes namespace re-exports (LIMITATION)');
+    assert.strictEqual(oldResult, expected, 'Old extension moves namespace re-exports after imports');
+    assert.strictEqual(newResult, expected, 'New extension moves namespace re-exports after imports');
   });
 
   // ============================================================================
@@ -402,10 +390,10 @@ const x: A = B;
   // A9: CRLF end-of-line parity
   // ============================================================================
 
-  test('A9: CRLF line endings preserved by both extensions', async () => {
+  test('A9: CRLF line endings preserved', async () => {
     const input = `import { Z } from './z';\r\nimport { A } from './a';\r\n\r\nconst x = A;\r\nconst y = Z;\r\n`;
 
-    // Expected: Both extensions preserve CRLF
+    // Expected: CRLF line endings are preserved
     const expected = `import { A } from './a';\r\nimport { Z } from './z';\r\n\r\nconst x = A;\r\nconst y = Z;\r\n`;
 
     const oldResult = await organizeImportsOld(input, { legacyMode: true });

src/imports/import-manager.ts

@@ -20,6 +20,7 @@ import { ImportGroup } from './import-grouping';
 export class ImportManager {
   private sourceFile!: SourceFile;
   private imports: Import[] = [];
+  private reExports: string[] = []; // Store re-export statements to preserve them
   private usedIdentifiers: Set<string> = new Set();
   private readonly eol: string;
 
@@ -122,6 +123,19 @@ export class ImportManager {
         }
       }
     }
+
+    // Extract re-export statements (export { X } from './m' or export * as ns from './m')
+    // These should be preserved and placed AFTER imports
+    const exportDeclarations = this.sourceFile.getExportDeclarations();
+    for (const exportDecl of exportDeclarations) {
+      // Only capture re-exports (those with moduleSpecifier)
+      // export { X } without 'from' is a local export, not a re-export
+      const moduleSpecifier = exportDecl.getModuleSpecifier();
+      if (moduleSpecifier) {
+        // Preserve the full re-export statement
+        this.reExports.push(exportDecl.getText());
+      }
+    }
   }
 
   /**
@@ -533,7 +547,11 @@ export class ImportManager {
     const importEquals = this.sourceFile.getStatements()
       .filter(stmt => Node.isImportEqualsDeclaration(stmt));
 
-    const allImports = [...importDeclarations, ...importEquals as any[]];
+    // Also include re-export declarations in the range (they'll be moved after imports)
+    const exportDeclarations = this.sourceFile.getExportDeclarations()
+      .filter(exportDecl => exportDecl.getModuleSpecifier() !== undefined);
+
+    const allImports = [...importDeclarations, ...importEquals as any[], ...exportDeclarations as any[]];
 
     if (allImports.length === 0) {
       return edits;
@@ -670,6 +688,12 @@ export class ImportManager {
         importText += this.eol;
       }
 
+      // Add re-export statements after imports (preserves export { X } from './m')
+      if (this.reExports.length > 0) {
+        importText += this.reExports.join(this.eol);
+        importText += this.eol;
+      }
+
       // Create the replace edit
       const replaceRange = new Range(
         this.document.lineAt(importSectionStartLine).range.start,

src/test/import-manager.edge-cases.test.ts

@@ -756,9 +756,10 @@ import { Y } from './m';
 const foo = Y;
 `;
 
-    const expected = `export { X } from './m';
-import { Y } from './m';
+    // Expected: Re-exports are moved AFTER imports (matching old TypeScript Hero behavior)
+    const expected = `import { Y } from './m';
 
+export { X } from './m';
 const foo = Y;
 `;
 
@@ -770,7 +771,7 @@ const foo = Y;
       await applyEditsToDocument(doc, edits);
 
       const result = doc.getText();
-      assert.strictEqual(result, expected, 'Re-exports and imports from same module must not merge');
+      assert.strictEqual(result, expected, 'Re-exports moved after imports, not merged with imports');
     } finally {
       await deleteTempDocument(doc);
     }