Skip to content

Commit 0ed1910

Browse files
committed
fix: resolve test failures from code audit fixes
- Fix overlapping TextEdit ranges when deleting leading blank lines (TC-100): Don't extend importSectionStartLine backwards when we've already created a separate delete edit for leading blanks - Fix test 12a & B6 expectations: specifierSort uses ASCII order (stringSort) matching old TypeScript Hero, not locale-aware sorting - Fix batch-organizer test path: resolve from project root to src/ instead of incorrectly reading from out/src/ - Add comprehensive test coverage for conflict-detector edge cases (codeActionsOnSave as boolean/string, vsCodeBuiltInEnabled state) All 389 tests now passing.
1 parent 07ee243 commit 0ed1910

12 files changed

Lines changed: 268 additions & 73 deletions

src/commands/batch-organizer.ts

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,18 @@ export class BatchOrganizer {
5555
return;
5656
}
5757

58+
// Warn user about large workspaces to prevent memory issues
59+
if (files.length > 1000) {
60+
const proceed = await window.showWarningMessage(
61+
`Mini TypeScript Hero: Found ${files.length} files. This may take a while and use significant memory. Continue?`,
62+
'Continue', 'Cancel'
63+
);
64+
if (proceed !== 'Continue') {
65+
this.logger.appendLine(`[BatchOrganizer] User cancelled operation with ${files.length} files`);
66+
return;
67+
}
68+
}
69+
5870
const result = await window.withProgress(
5971
{
6072
location: ProgressLocation.Window,
@@ -148,7 +160,10 @@ export class BatchOrganizer {
148160
}
149161

150162
// Calculate relative path from workspace root to folder
151-
const relativePath = folderUri.fsPath.substring(workspaceFolder.uri.fsPath.length + 1);
163+
// Normalize to forward slashes for glob matching (Windows uses backslashes)
164+
const relativePath = folderUri.fsPath
165+
.substring(workspaceFolder.uri.fsPath.length + 1)
166+
.replace(/\\/g, '/');
152167
// Handle case where folder IS the workspace root (relative path is empty)
153168
const include = relativePath
154169
? `${relativePath}/**/*.{ts,tsx,js,jsx}`
@@ -204,7 +219,10 @@ export class BatchOrganizer {
204219
const excludePatterns = this.getExcludePatterns(workspaceFolder.uri);
205220

206221
// Convert file URI to path relative to workspace root
207-
const relativePath = fileUri.fsPath.substring(workspaceFolder.uri.fsPath.length + 1);
222+
// Normalize to forward slashes for glob matching (Windows uses backslashes)
223+
const relativePath = fileUri.fsPath
224+
.substring(workspaceFolder.uri.fsPath.length + 1)
225+
.replace(/\\/g, '/');
208226

209227
// Check if file matches any exclude pattern
210228
for (const pattern of excludePatterns) {
@@ -229,7 +247,10 @@ export class BatchOrganizer {
229247
}
230248

231249
// Convert file URI to path relative to workspace root
232-
const relativePath = fileUri.fsPath.substring(workspaceFolder.uri.fsPath.length + 1);
250+
// Normalize to forward slashes for glob matching (Windows uses backslashes)
251+
const relativePath = fileUri.fsPath
252+
.substring(workspaceFolder.uri.fsPath.length + 1)
253+
.replace(/\\/g, '/');
233254

234255
// Check if file matches any exclude pattern
235256
for (const pattern of excludePatterns) {
@@ -327,13 +348,23 @@ export class BatchOrganizer {
327348

328349
// CRITICAL: Save all modified documents to disk!
329350
// workspace.applyEdit() only modifies in-memory documents, doesn't save!
351+
const saveFailed: string[] = [];
330352
for (const [fileUriStr] of workspaceEdit.entries()) {
331-
const doc = workspace.textDocuments.find(d => d.uri.toString() === fileUriStr.toString());
353+
const doc = workspace.textDocuments.find(d => d.uri.fsPath === fileUriStr.fsPath);
332354
if (doc && !doc.isUntitled && doc.isDirty) {
333-
await doc.save();
355+
try {
356+
await doc.save();
357+
} catch (saveError) {
358+
saveFailed.push(doc.fileName);
359+
errors++;
360+
this.logger.appendLine(`[BatchOrganizer] Failed to save ${doc.fileName}: ${saveError}`);
361+
}
334362
}
335363
}
336-
this.logger.appendLine(`[BatchOrganizer] Saved ${processed} files to disk`);
364+
if (saveFailed.length > 0) {
365+
window.showWarningMessage(`Mini TypeScript Hero: Failed to save ${saveFailed.length} file(s). Check output for details.`);
366+
}
367+
this.logger.appendLine(`[BatchOrganizer] Saved ${processed - saveFailed.length} files to disk`);
337368
} else {
338369
window.showErrorMessage('Mini TypeScript Hero: Failed to apply some edits');
339370
this.logger.appendLine('[BatchOrganizer] Failed to apply edits');

src/configuration/conflict-detector.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,13 @@ export function detectConflicts(): ConflictInfo {
6666
// Only a conflict if OUR organizeOnSave is also enabled
6767
// Ignore "never" and false values (explicitly disabled)
6868
const editorConfig = workspace.getConfiguration('editor');
69-
const codeActionsOnSave = editorConfig.get('codeActionsOnSave') as Record<string, boolean | string> | undefined;
70-
const organizeImportsValue: boolean | string | undefined = codeActionsOnSave?.['source.organizeImports'];
69+
const codeActionsOnSaveRaw = editorConfig.get('codeActionsOnSave');
70+
// Handle different types - codeActionsOnSave could be object, boolean, string, or undefined
71+
let organizeImportsValue: boolean | string | undefined;
72+
if (codeActionsOnSaveRaw && typeof codeActionsOnSaveRaw === 'object') {
73+
organizeImportsValue = (codeActionsOnSaveRaw as Record<string, boolean | string>)['source.organizeImports'];
74+
}
75+
// If codeActionsOnSave is not an object, organizeImportsValue stays undefined (no conflict)
7176
const vsCodeBuiltInEnabled = organizeImportsValue !== false && organizeImportsValue !== 'never' && organizeImportsValue !== undefined;
7277
const vsCodeBuiltInConflict = vsCodeBuiltInEnabled && ourOrganizeOnSave;
7378

@@ -78,7 +83,7 @@ export function detectConflicts(): ConflictInfo {
7883
return {
7984
oldExtensionInstalled,
8085
oldOrganizeOnSaveEnabled,
81-
vsCodeBuiltInEnabled: vsCodeBuiltInConflict,
86+
vsCodeBuiltInEnabled, // Return actual enabled state, not conflict state
8287
ourOrganizeOnSaveEnabled: ourOrganizeOnSave,
8388
conflicts,
8489
};

src/configuration/imports-config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ export class ImportsConfig {
130130
const insertSpaces = editorConfig.get<boolean>('insertSpaces', true);
131131

132132
// Check if tabSize is explicitly configured (vs just VS Code's built-in default of 4)
133+
// Note: EditorConfig values also appear here, which is correct - we respect EditorConfig.
133134
const tabSizeInspect = editorConfig.inspect<number>('tabSize');
134135
let tabSize: number;
135136

src/configuration/settings-migration.ts

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,35 @@ const SETTINGS_TO_MIGRATE = [
2323
'grouping',
2424
];
2525

26+
/**
27+
* Expected types for each setting. Used to validate migrated values.
28+
*/
29+
const SETTING_TYPES: Record<string, string> = {
30+
'insertSpaceBeforeAndAfterImportBraces': 'boolean',
31+
'insertSemicolons': 'boolean',
32+
'removeTrailingIndex': 'boolean',
33+
'stringQuoteStyle': 'string',
34+
'multiLineWrapThreshold': 'number',
35+
'multiLineTrailingComma': 'boolean',
36+
'disableImportRemovalOnOrganize': 'boolean',
37+
'disableImportsSorting': 'boolean',
38+
'organizeOnSave': 'boolean',
39+
'organizeSortsByFirstSpecifier': 'boolean',
40+
'ignoredFromRemoval': 'object', // array
41+
'grouping': 'object', // array
42+
};
43+
44+
/**
45+
* Validates that a setting value has the expected type.
46+
*/
47+
function isValidSettingType(setting: string, value: unknown): boolean {
48+
const expectedType = SETTING_TYPES[setting];
49+
if (!expectedType) {
50+
return true; // Unknown setting, allow migration
51+
}
52+
return typeof value === expectedType;
53+
}
54+
2655
/**
2756
* Migrates settings from the original TypeScript Hero extension to Mini TypeScript Hero.
2857
*
@@ -90,23 +119,24 @@ async function performMigration(): Promise<number> {
90119

91120
// Migrate from each configuration level where it was set
92121
// Priority: workspace > workspaceFolder > global
122+
// Only migrate if the value has the correct type
93123

94124
// Migrate workspace settings
95-
if (inspect.workspaceValue !== undefined) {
125+
if (inspect.workspaceValue !== undefined && isValidSettingType(setting, inspect.workspaceValue)) {
96126
await newConfig.update(setting, inspect.workspaceValue, ConfigurationTarget.Workspace);
97127
migratedCount++;
98128
migratedWorkspaceCount++;
99129
}
100130

101131
// Migrate workspace folder settings
102-
if (inspect.workspaceFolderValue !== undefined) {
132+
if (inspect.workspaceFolderValue !== undefined && isValidSettingType(setting, inspect.workspaceFolderValue)) {
103133
await newConfig.update(setting, inspect.workspaceFolderValue, ConfigurationTarget.WorkspaceFolder);
104134
migratedCount++;
105135
migratedWorkspaceFolderCount++;
106136
}
107137

108138
// Migrate global (user) settings
109-
if (inspect.globalValue !== undefined) {
139+
if (inspect.globalValue !== undefined && isValidSettingType(setting, inspect.globalValue)) {
110140
await newConfig.update(setting, inspect.globalValue, ConfigurationTarget.Global);
111141
migratedCount++;
112142
migratedGlobalCount++;

src/extension.ts

Lines changed: 38 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -277,31 +277,31 @@ export async function activate(context: ExtensionContext): Promise<void> {
277277

278278
// Register command: Toggle legacy mode
279279
const toggleLegacyModeCommand = commands.registerCommand('miniTypescriptHero.toggleLegacyMode', async () => {
280-
const importsConfig = workspace.getConfiguration('miniTypescriptHero.imports');
281-
const currentValue = importsConfig.get<boolean>('legacyMode', false);
282-
const newValue = !currentValue;
283-
284-
// Determine best scope to update
285-
// Priority: WorkspaceFolder > Workspace > Global
286-
const inspection = importsConfig.inspect('legacyMode');
287-
let target = ConfigurationTarget.Global;
288-
let scopeName = 'User (Global)';
289-
290-
if (workspace.workspaceFolders && workspace.workspaceFolders.length > 0) {
291-
if (inspection?.workspaceFolderValue !== undefined) {
292-
target = ConfigurationTarget.WorkspaceFolder;
293-
scopeName = 'Workspace Folder';
294-
} else if (inspection?.workspaceValue !== undefined) {
295-
target = ConfigurationTarget.Workspace;
296-
scopeName = 'Workspace';
297-
} else if (workspace.workspaceFolders.length === 1) {
298-
// Single workspace folder - prefer Workspace scope
299-
target = ConfigurationTarget.Workspace;
300-
scopeName = 'Workspace';
280+
try {
281+
const importsConfig = workspace.getConfiguration('miniTypescriptHero.imports');
282+
const currentValue = importsConfig.get<boolean>('legacyMode', false);
283+
const newValue = !currentValue;
284+
285+
// Determine best scope to update
286+
// Priority: WorkspaceFolder > Workspace > Global
287+
const inspection = importsConfig.inspect('legacyMode');
288+
let target = ConfigurationTarget.Global;
289+
let scopeName = 'User (Global)';
290+
291+
if (workspace.workspaceFolders && workspace.workspaceFolders.length > 0) {
292+
if (inspection?.workspaceFolderValue !== undefined) {
293+
target = ConfigurationTarget.WorkspaceFolder;
294+
scopeName = 'Workspace Folder';
295+
} else if (inspection?.workspaceValue !== undefined) {
296+
target = ConfigurationTarget.Workspace;
297+
scopeName = 'Workspace';
298+
} else if (workspace.workspaceFolders.length === 1) {
299+
// Single workspace folder - prefer Workspace scope
300+
target = ConfigurationTarget.Workspace;
301+
scopeName = 'Workspace';
302+
}
301303
}
302-
}
303304

304-
try {
305305
await importsConfig.update('legacyMode', newValue, target);
306306
const statusText = newValue ? 'enabled' : 'disabled';
307307
window.showInformationMessage(
@@ -328,17 +328,23 @@ export async function activate(context: ExtensionContext): Promise<void> {
328328

329329
// Register command: Organize imports in folder
330330
const organizeFolderCommand = commands.registerCommand('miniTypescriptHero.imports.organizeFolder', async (clickedFolder?: Uri) => {
331-
if (!clickedFolder) {
332-
// Fallback: use workspace root if called from command palette
333-
if (!workspace.workspaceFolders || workspace.workspaceFolders.length === 0) {
334-
window.showWarningMessage('Mini TypeScript Hero: No workspace folder open');
335-
return;
331+
try {
332+
if (!clickedFolder) {
333+
// Fallback: use workspace root if called from command palette
334+
if (!workspace.workspaceFolders || workspace.workspaceFolders.length === 0) {
335+
window.showWarningMessage('Mini TypeScript Hero: No workspace folder open');
336+
return;
337+
}
338+
clickedFolder = workspace.workspaceFolders[0].uri;
336339
}
337-
clickedFolder = workspace.workspaceFolders[0].uri;
338-
}
339340

340-
outputChannel.appendLine(`Mini TypeScript Hero: Running organize imports in folder: ${clickedFolder.fsPath}`);
341-
await batchOrganizer.organizeFolder(clickedFolder);
341+
outputChannel.appendLine(`Mini TypeScript Hero: Running organize imports in folder: ${clickedFolder.fsPath}`);
342+
await batchOrganizer.organizeFolder(clickedFolder);
343+
} catch (error) {
344+
const errorMessage = error instanceof Error ? error.message : String(error);
345+
window.showErrorMessage(`Mini TypeScript Hero: Failed to organize folder: ${errorMessage}`);
346+
outputChannel.appendLine(`Mini TypeScript Hero: Error organizing folder: ${errorMessage}`);
347+
}
342348
});
343349
context.subscriptions.push(organizeFolderCommand);
344350

src/imports/import-manager.ts

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,9 @@ export class ImportManager {
179179

180180
// Extract re-export statements (export { X } from './m' or export * as ns from './m')
181181
// These should be preserved and placed AFTER imports
182+
//
183+
// KNOWN LIMITATION: Re-exports are preserved in original file order, not sorted/grouped.
184+
// Sorting re-exports could be added in a future version if there's demand.
182185
const exportDeclarations = this.sourceFile.getExportDeclarations();
183186
for (const exportDecl of exportDeclarations) {
184187
// Only capture re-exports (those with moduleSpecifier)
@@ -550,7 +553,12 @@ export class ImportManager {
550553
for (const imp of keep) {
551554
// In modern mode, include isTypeOnly in the grouping key
552555
// This prevents merging type-only imports with value imports
553-
const typePrefix = (!isLegacy && imp instanceof NamedImport && imp.isTypeOnly) ? 'type:' : '';
556+
// Check both NamedImport and NamespaceImport for isTypeOnly flag
557+
const isTypeOnlyImport = !isLegacy && (
558+
(imp instanceof NamedImport && imp.isTypeOnly) ||
559+
(imp instanceof NamespaceImport && imp.isTypeOnly)
560+
);
561+
const typePrefix = isTypeOnlyImport ? 'type:' : '';
554562
const groupKey = typePrefix + imp.libraryName;
555563

556564
if (!byLibrary.has(groupKey)) {
@@ -726,25 +734,35 @@ export class ImportManager {
726734
// Get the position info including blank lines before imports
727735
const { blankLinesBefore, hasHeader, hasLeadingBlanks, headerStartLine } = this.getImportInsertPosition();
728736

729-
// Delete leading blank lines before header (if any) as a separate edit
730-
if (hasLeadingBlanks && headerStartLine > 0) {
731-
const leadingBlanksRange = new Range(
732-
new Position(0, 0),
733-
new Position(headerStartLine, 0),
734-
);
735-
edits.push(TextEdit.delete(leadingBlanksRange));
736-
}
737-
738737
// Calculate the full range of imports to replace (excluding any header)
739738
const firstImport = allImports[0];
740739
const lastImport = allImports[allImports.length - 1];
741740

742741
let importSectionStartLine = firstImport.getStartLineNumber() - 1; // Convert to 0-indexed
743742
let importSectionEndLine = lastImport.getEndLineNumber() - 1;
744743

744+
// Delete leading blank lines before header or imports (if any) as a separate edit
745+
// When hasLeadingBlanks is true:
746+
// - If headerStartLine > 0: There's a header, delete blanks from line 0 to headerStartLine
747+
// - If headerStartLine === -1: No header, delete blanks from line 0 to importSectionStartLine
748+
if (hasLeadingBlanks) {
749+
const deleteToLine = headerStartLine > 0 ? headerStartLine : importSectionStartLine;
750+
if (deleteToLine > 0) {
751+
const leadingBlanksRange = new Range(
752+
new Position(0, 0),
753+
new Position(deleteToLine, 0),
754+
);
755+
edits.push(TextEdit.delete(leadingBlanksRange));
756+
}
757+
}
758+
745759
// Extract comments between imports (old TypeScript Hero moves them after imports)
746760
// Only extract standalone comment lines that are BETWEEN import declarations,
747761
// not lines that are WITHIN a multi-line import declaration
762+
//
763+
// KNOWN LIMITATION: Comments INSIDE multiline import braces are not preserved.
764+
// Example: `import { Foo, /* comment */ Bar } from 'lib'` - the comment is lost.
765+
// This is complex to fix and is an edge case. Standalone comments between imports ARE preserved.
748766
const commentsBetweenImports: string[] = [];
749767
for (let i = importSectionStartLine; i <= importSectionEndLine; i++) {
750768
const lineNumber = i + 1; // Convert to 1-indexed for comparison with ts-morph
@@ -773,8 +791,11 @@ export class ImportManager {
773791
}
774792
}
775793

776-
// Include blank lines before first import (but not header)
777-
if (blankLinesBefore > 0) {
794+
// Include blank lines before first import (but not header) in the replace range.
795+
// IMPORTANT: Only do this if we did NOT create a separate delete edit for leading blanks!
796+
// When hasLeadingBlanks is true, we already deleted lines 0 to importSectionStartLine,
797+
// so we must NOT extend importSectionStartLine backwards (would create overlapping ranges).
798+
if (blankLinesBefore > 0 && !hasLeadingBlanks) {
778799
importSectionStartLine = Math.max(0, importSectionStartLine - blankLinesBefore);
779800
}
780801

src/imports/import-organizer.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@ export class ImportOrganizer implements Disposable {
5757
this.runningOrganizes.add(key);
5858
event.waitUntil(
5959
this.organizeImportsForDocument(event.document)
60+
.catch(err => {
61+
this.logger.appendLine(`[ImportOrganizer] Error in organize-on-save: ${err}`);
62+
return []; // Return empty edits on error to allow save to proceed
63+
})
6064
.finally(() => {
6165
this.runningOrganizes.delete(key);
6266
}),
@@ -204,7 +208,10 @@ export class ImportOrganizer implements Disposable {
204208
const allPatterns = [...defaultPatterns, ...userPatterns];
205209

206210
// Convert file URI to path relative to workspace root
207-
const relativePath = fileUri.fsPath.substring(workspaceFolder.uri.fsPath.length + 1);
211+
// Normalize to forward slashes for glob matching (Windows uses backslashes)
212+
const relativePath = fileUri.fsPath
213+
.substring(workspaceFolder.uri.fsPath.length + 1)
214+
.replace(/\\/g, '/');
208215

209216
// Check if file matches any exclude pattern
210217
for (const pattern of allPatterns) {

src/imports/import-utilities.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,11 @@ function getImportFirstSpecifier(imp: Import): string {
7676
}
7777

7878
/**
79-
* Order specifiers by name (case-insensitive, locale-aware).
79+
* Order specifiers by name.
80+
* Uses stringSort (not localeStringSort) to match old TypeScript Hero behavior.
8081
*/
8182
export function specifierSort(i1: SymbolSpecifier, i2: SymbolSpecifier): number {
82-
return localeStringSort(i1.specifier, i2.specifier);
83+
return stringSort(i1.specifier, i2.specifier);
8384
}
8485

8586
/**

0 commit comments

Comments
 (0)