Skip to content

Commit 2f9d1cf

Browse files
committed
fix: address 10 bugs from second comprehensive audit
Production bugs fixed: - B1: Re-exports (export { Foo } from './bar') no longer falsely prevent removal of unrelated local imports with the same name - B2: Regex import group now parses /pattern/flags syntax correctly - B3: Settings migration now iterates workspace folders to migrate folder-level settings in multi-root workspaces - B4: Conflict detector checks all scopes including workspace folders - B5: "Disable for Me" now finds and updates only the scope where source.organizeImports is actually set, avoiding merged value clobbering - B6: Invalid order values (e.g., "DESC") are normalized to valid enum - B7: hasWarnedKey only set after successful auto-fix, not when skipped - B8: Unused namespace stripped from import Default, * as ns (converts to default-only import when only default is used) - B9: Re-entrancy guard added to manual organize command - B10: Remove double disposal of outputChannel in deactivate() Also fixes wrong specifier order in blog post and README examples (Component, OnInit, inject → Component, inject, OnInit).
1 parent d888924 commit 2f9d1cf

10 files changed

Lines changed: 178 additions & 68 deletions

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ import { BookList } from './components/book-list';
4545
**After** pressing `Ctrl+Alt+O` (or `Cmd+Alt+O` on macOS):
4646

4747
```typescript
48-
import { Component, OnInit, inject } from '@angular/core';
48+
import { Component, inject, OnInit } from '@angular/core';
4949
import { Router } from '@angular/router';
5050
import { map, switchMap } from 'rxjs/operators';
5151

blog-post.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { BookList } from './components/book-list';
2828
**After** pressing `Ctrl+Alt+O` (or `Cmd+Alt+O` on macOS):
2929

3030
```typescript
31-
import { Component, OnInit, inject } from '@angular/core';
31+
import { Component, inject, OnInit } from '@angular/core';
3232
import { Router } from '@angular/router';
3333
import { map, switchMap } from 'rxjs/operators';
3434

src/configuration/conflict-detector.ts

Lines changed: 51 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,9 @@ export interface ConflictInfo {
3838
export function detectConflicts(): ConflictInfo {
3939
const conflicts: string[] = [];
4040

41-
// Read our own organizeOnSave setting to determine if on-save conflicts are real
42-
const ourConfig = workspace.getConfiguration('miniTypescriptHero.imports');
43-
const ourOrganizeOnSave = ourConfig.get<boolean>('organizeOnSave', false);
41+
// Read our own organizeOnSave setting to determine if on-save conflicts are real.
42+
// Check all scopes including workspace folders (unscoped read misses folder-level settings).
43+
const ourOrganizeOnSave = isBooleanSettingEnabled('miniTypescriptHero.imports', 'organizeOnSave');
4444

4545
// Check 1: Old TypeScript Hero extension installed
4646
const oldExtension = extensions.getExtension(OLD_EXTENSION_ID);
@@ -52,8 +52,7 @@ export function detectConflicts(): ConflictInfo {
5252

5353
// Check 2: Old TypeScript Hero organize-on-save enabled
5454
// Only a conflict if OUR organizeOnSave is also enabled
55-
const oldTsHeroConfig = workspace.getConfiguration('typescriptHero.imports');
56-
const oldOrganizeOnSave = oldTsHeroConfig.get<boolean>('organizeOnSave', false);
55+
const oldOrganizeOnSave = isBooleanSettingEnabled('typescriptHero.imports', 'organizeOnSave');
5756
const oldOrganizeOnSaveEnabled = oldOrganizeOnSave && ourOrganizeOnSave;
5857

5958
if (oldOrganizeOnSaveEnabled) {
@@ -64,16 +63,7 @@ export function detectConflicts(): ConflictInfo {
6463

6564
// Check 3: VS Code built-in organize imports enabled
6665
// Only a conflict if OUR organizeOnSave is also enabled
67-
// Ignore "never" and false values (explicitly disabled)
68-
const editorConfig = workspace.getConfiguration('editor');
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)
76-
const vsCodeBuiltInEnabled = organizeImportsValue !== false && organizeImportsValue !== 'never' && organizeImportsValue !== undefined;
66+
const vsCodeBuiltInEnabled = isOrganizeImportsCodeActionEnabled();
7767
const vsCodeBuiltInConflict = vsCodeBuiltInEnabled && ourOrganizeOnSave;
7868

7969
if (vsCodeBuiltInConflict) {
@@ -88,3 +78,49 @@ export function detectConflicts(): ConflictInfo {
8878
conflicts,
8979
};
9080
}
81+
82+
/**
83+
* Check if a boolean setting is enabled in any scope (global, workspace, or any workspace folder).
84+
* Unscoped getConfiguration().get() misses workspace-folder-level values in multi-root workspaces.
85+
*/
86+
function isBooleanSettingEnabled(section: string, key: string): boolean {
87+
const config = workspace.getConfiguration(section);
88+
const inspect = config.inspect<boolean>(key);
89+
if (inspect?.globalValue || inspect?.workspaceValue) {
90+
return true;
91+
}
92+
for (const folder of workspace.workspaceFolders ?? []) {
93+
const fi = workspace.getConfiguration(section, folder.uri).inspect<boolean>(key);
94+
if (fi?.workspaceFolderValue) {
95+
return true;
96+
}
97+
}
98+
return false;
99+
}
100+
101+
/**
102+
* Check if editor.codeActionsOnSave has source.organizeImports enabled in any scope.
103+
*/
104+
function isOrganizeImportsCodeActionEnabled(): boolean {
105+
const config = workspace.getConfiguration('editor');
106+
const inspect = config.inspect('codeActionsOnSave');
107+
108+
const checkValue = (value: unknown): boolean => {
109+
if (value && typeof value === 'object') {
110+
const oi = (value as Record<string, boolean | string>)['source.organizeImports'];
111+
return oi !== false && oi !== 'never' && oi !== undefined;
112+
}
113+
return false;
114+
};
115+
116+
if (checkValue(inspect?.globalValue) || checkValue(inspect?.workspaceValue)) {
117+
return true;
118+
}
119+
for (const folder of workspace.workspaceFolders ?? []) {
120+
const fi = workspace.getConfiguration('editor', folder.uri).inspect('codeActionsOnSave');
121+
if (checkValue(fi?.workspaceFolderValue)) {
122+
return true;
123+
}
124+
}
125+
return false;
126+
}

src/configuration/settings-migration.ts

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -109,32 +109,21 @@ async function performMigration(): Promise<number> {
109109
let migratedWorkspaceCount = 0;
110110
let migratedWorkspaceFolderCount = 0;
111111

112+
// Step 1: Migrate global and workspace-level settings
112113
for (const setting of SETTINGS_TO_MIGRATE) {
113-
// Check if old setting exists (has been configured by user)
114114
const inspect = oldConfig.inspect(setting);
115115

116116
if (!inspect) {
117117
continue;
118118
}
119119

120-
// Migrate from each configuration level where it was set
121-
// Priority: workspace > workspaceFolder > global
122-
// Only migrate if the value has the correct type
123-
124120
// Migrate workspace settings
125121
if (inspect.workspaceValue !== undefined && isValidSettingType(setting, inspect.workspaceValue)) {
126122
await newConfig.update(setting, inspect.workspaceValue, ConfigurationTarget.Workspace);
127123
migratedCount++;
128124
migratedWorkspaceCount++;
129125
}
130126

131-
// Migrate workspace folder settings
132-
if (inspect.workspaceFolderValue !== undefined && isValidSettingType(setting, inspect.workspaceFolderValue)) {
133-
await newConfig.update(setting, inspect.workspaceFolderValue, ConfigurationTarget.WorkspaceFolder);
134-
migratedCount++;
135-
migratedWorkspaceFolderCount++;
136-
}
137-
138127
// Migrate global (user) settings
139128
if (inspect.globalValue !== undefined && isValidSettingType(setting, inspect.globalValue)) {
140129
await newConfig.update(setting, inspect.globalValue, ConfigurationTarget.Global);
@@ -143,6 +132,27 @@ async function performMigration(): Promise<number> {
143132
}
144133
}
145134

135+
// Step 2: Migrate workspace-folder-level settings
136+
// getConfiguration() without a URI cannot see workspaceFolderValue in multi-root,
137+
// so we iterate over each workspace folder explicitly.
138+
for (const folder of workspace.workspaceFolders ?? []) {
139+
const oldFolderConfig = workspace.getConfiguration(OLD_SECTION, folder.uri);
140+
const newFolderConfig = workspace.getConfiguration(NEW_SECTION, folder.uri);
141+
142+
for (const setting of SETTINGS_TO_MIGRATE) {
143+
const inspect = oldFolderConfig.inspect(setting);
144+
if (!inspect || inspect.workspaceFolderValue === undefined) {
145+
continue;
146+
}
147+
148+
if (isValidSettingType(setting, inspect.workspaceFolderValue)) {
149+
await newFolderConfig.update(setting, inspect.workspaceFolderValue, ConfigurationTarget.WorkspaceFolder);
150+
migratedCount++;
151+
migratedWorkspaceFolderCount++;
152+
}
153+
}
154+
}
155+
146156
// For migrated users: Enable legacyMode for formatting backward compatibility
147157
// Write legacyMode to the SAME scope(s) where old settings were found
148158
if (migratedCount > 0) {

src/extension.ts

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -124,38 +124,50 @@ async function checkForConflicts(
124124
// Automatically disable VSCode's built-in organize imports
125125
try {
126126
const editorConfig = workspace.getConfiguration('editor');
127-
const currentActions = editorConfig.get('codeActionsOnSave') as Record<string, boolean | string> | undefined;
128-
129-
if (currentActions) {
130-
// Create new object with source.organizeImports set to false
131-
const newActions = { ...currentActions };
132-
newActions['source.organizeImports'] = false;
133-
134-
// Determine which scope to update (most specific scope wins)
135-
const inspection = editorConfig.inspect('codeActionsOnSave');
136-
let target = ConfigurationTarget.Global;
137-
138-
if (inspection?.workspaceFolderValue) {
139-
target = ConfigurationTarget.WorkspaceFolder;
140-
} else if (inspection?.workspaceValue) {
141-
target = ConfigurationTarget.Workspace;
127+
const inspection = editorConfig.inspect('codeActionsOnSave');
128+
let updated = false;
129+
130+
// Check each scope individually and update only where source.organizeImports is set.
131+
// This avoids clobbering other scopes by writing the merged effective value.
132+
type ScopeValue = Record<string, boolean | string> | undefined;
133+
const scopes: Array<{ value: ScopeValue; target: ConfigurationTarget }> = [
134+
{ value: inspection?.globalValue as ScopeValue, target: ConfigurationTarget.Global },
135+
{ value: inspection?.workspaceValue as ScopeValue, target: ConfigurationTarget.Workspace },
136+
];
137+
138+
// Also check workspace folders (unscoped inspect misses folder-level values)
139+
for (const folder of workspace.workspaceFolders ?? []) {
140+
const fi = workspace.getConfiguration('editor', folder.uri).inspect('codeActionsOnSave');
141+
if (fi?.workspaceFolderValue && typeof fi.workspaceFolderValue === 'object') {
142+
scopes.push({ value: fi.workspaceFolderValue as ScopeValue, target: ConfigurationTarget.WorkspaceFolder });
142143
}
144+
}
143145

144-
await editorConfig.update('codeActionsOnSave', newActions, target);
146+
for (const scope of scopes) {
147+
if (scope.value && typeof scope.value === 'object' &&
148+
'source.organizeImports' in scope.value &&
149+
scope.value['source.organizeImports'] !== false &&
150+
scope.value['source.organizeImports'] !== 'never') {
151+
const newActions = { ...scope.value, 'source.organizeImports': false };
152+
await editorConfig.update('codeActionsOnSave', newActions, scope.target);
153+
updated = true;
154+
}
155+
}
145156

157+
if (updated) {
146158
window.showInformationMessage(
147159
'Mini TypeScript Hero: VSCode built-in organize imports has been disabled. ' +
148160
'You can re-enable it anytime in Settings.'
149161
);
150162
outputChannel.appendLine('Mini TypeScript Hero: Successfully disabled VSCode built-in organize imports');
163+
// Mark as warned only after successful fix
164+
await context.globalState.update(hasWarnedKey, true);
151165
}
152166
} catch (error) {
153167
const errorMessage = error instanceof Error ? error.message : String(error);
154168
window.showErrorMessage(`Mini TypeScript Hero: Failed to disable VSCode built-in: ${errorMessage}`);
155169
outputChannel.appendLine(`Mini TypeScript Hero: Error disabling VSCode built-in: ${errorMessage}`);
156170
}
157-
// Mark as warned - conflict resolved
158-
await context.globalState.update(hasWarnedKey, true);
159171
} else if (selection === 'Open Extensions') {
160172
// Open Extensions sidebar so user can disable old TypeScript Hero manually
161173
// NOTE: Using 'workbench.view.extensions' instead of 'workbench.extensions.action.showInstalledExtensions'
@@ -367,8 +379,5 @@ export async function activate(context: ExtensionContext): Promise<void> {
367379
* Deactivate the extension.
368380
*/
369381
export function deactivate(): void {
370-
if (outputChannel) {
371-
outputChannel.appendLine('Mini TypeScript Hero: Deactivating extension');
372-
outputChannel.dispose();
373-
}
382+
// outputChannel is disposed automatically via context.subscriptions
374383
}

src/imports/import-grouping/import-group-setting-parser.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,9 @@ export class ImportGroupSettingParser {
5050
identifier = setting;
5151
} else {
5252
identifier = setting.identifier;
53-
order = setting.order;
53+
// Normalize order to valid enum value (case-insensitive)
54+
const rawOrder = String(setting.order || '').toLowerCase();
55+
order = rawOrder === ImportGroupOrder.Desc ? ImportGroupOrder.Desc : ImportGroupOrder.Asc;
5456
}
5557

5658
if (REGEX_REGEX_GROUP.test(identifier)) {

src/imports/import-grouping/regex-import-group.ts

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,15 +34,13 @@ export class RegexImportGroup implements ImportGroup {
3434
public readonly order: ImportGroupOrder = ImportGroupOrder.Asc,
3535
) {
3636
// Compile regex once in constructor for performance
37-
// Strip surrounding slashes if present (e.g., '/^@angular/' -> '^@angular')
38-
let regexString = this.regex;
39-
regexString = regexString.startsWith('/')
40-
? regexString.substring(1)
41-
: regexString;
42-
regexString = regexString.endsWith('/')
43-
? regexString.substring(0, regexString.length - 1)
44-
: regexString;
45-
this.compiledRegex = new RegExp(regexString);
37+
// Parse /pattern/flags syntax (e.g., '/^@angular/i' -> pattern='^@angular', flags='i')
38+
const slashMatch = this.regex.match(/^\/(.+)\/([gimsuy]*)$/);
39+
if (slashMatch) {
40+
this.compiledRegex = new RegExp(slashMatch[1], slashMatch[2]);
41+
} else {
42+
this.compiledRegex = new RegExp(this.regex);
43+
}
4644
}
4745

4846
public reset(): void {

src/imports/import-manager.ts

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -257,9 +257,15 @@ export class ImportManager {
257257
}
258258
});
259259

260-
// Step 2: Handle re-exported symbols (export { Foo } or export default Foo)
261-
// These must be kept even if not used in the file itself
260+
// Step 2: Handle locally re-exported symbols (export { Foo } or export default Foo)
261+
// These must be kept even if not used in the file itself.
262+
// Only mark names from LOCAL exports (no moduleSpecifier). Re-exports like
263+
// `export { Foo } from './bar'` reference symbols from the other module,
264+
// not local identifiers — they must not prevent removal of unrelated local imports.
262265
this.sourceFile.getExportDeclarations().forEach(exportDecl => {
266+
if (exportDecl.getModuleSpecifier()) {
267+
return;
268+
}
263269
const namedExports = exportDecl.getNamedExports();
264270
namedExports.forEach(named => {
265271
this.usedIdentifiers.add(named.getName());
@@ -292,6 +298,14 @@ export class ImportManager {
292298
continue;
293299
}
294300

301+
// Skip if this identifier is part of a re-export (export { ... } from '...')
302+
// Re-exports reference symbols from the other module, not local identifiers.
303+
// Local exports (export { Foo }) are handled by Step 2.
304+
const ancestorExportDecl = identifier.getFirstAncestorByKind(SyntaxKind.ExportDeclaration);
305+
if (ancestorExportDecl?.getModuleSpecifier()) {
306+
continue;
307+
}
308+
295309
// Skip if this identifier IS the name being declared (not usages in the declaration)
296310
const parent = identifier.getParent();
297311
if (Node.isClassDeclaration(parent) && identifier === parent.getNameNode()) {
@@ -498,11 +512,16 @@ export class ImportManager {
498512
const nsUsed = this.usedIdentifiers.has(imp.alias);
499513
const defaultUsed = !!imp.defaultAlias && this.usedIdentifiers.has(imp.defaultAlias);
500514
if (nsUsed || defaultUsed) {
501-
// Strip unused default if only namespace is used
502515
if (imp.defaultAlias && !defaultUsed) {
516+
// Strip unused default, keep namespace
503517
keep.push(new NamespaceImport(
504518
imp.libraryName, imp.alias, undefined, imp.isTypeOnly, imp.attributes,
505519
));
520+
} else if (!nsUsed && defaultUsed) {
521+
// Strip unused namespace, convert to default-only import
522+
keep.push(new NamedImport(
523+
imp.libraryName, [], imp.defaultAlias, imp.isTypeOnly, imp.attributes,
524+
));
506525
} else {
507526
keep.push(imp);
508527
}

src/imports/import-organizer.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,12 @@ export class ImportOrganizer implements Disposable {
8989
return;
9090
}
9191

92+
// Re-entrancy guard (same as organize-on-save)
93+
const key = editor.document.uri.toString();
94+
if (this.runningOrganizes.has(key)) {
95+
return;
96+
}
97+
9298
if (!this.isSupportedLanguage(editor.document.languageId)) {
9399
window.showWarningMessage(
94100
`Mini TypeScript Hero: Organize imports is not supported for ${editor.document.languageId} files`,
@@ -105,6 +111,7 @@ export class ImportOrganizer implements Disposable {
105111
return;
106112
}
107113

114+
this.runningOrganizes.add(key);
108115
try {
109116
this.logger.appendLine(`[ImportOrganizer] Organizing imports for ${editor.document.fileName}`);
110117

@@ -141,6 +148,8 @@ export class ImportOrganizer implements Disposable {
141148
if (error instanceof Error && error.stack) {
142149
this.logger.appendLine(error.stack);
143150
}
151+
} finally {
152+
this.runningOrganizes.delete(key);
144153
}
145154
}
146155

0 commit comments

Comments
 (0)