Skip to content

Commit ae68128

Browse files
committed
fix: address 10 bugs from fifth comprehensive audit
B1: Strip specifier-level type modifiers in legacy mode output B2: legacyMode guard now checks each workspace folder individually B3: Migration flag set in finally block to prevent retry loop B4: "Disable for Me" button only shown for actual conflicts B5: Old extension on-save conflict no longer suppressed B6: isStandaloneComment no longer matches non-comment * lines B7: Invalid user regex gracefully falls back instead of crashing B8: Regex flag validation supports ES2022/2024 d and v flags B9: Error message passed to super() instead of set after B10: Renamed isOldExtensionActive to isOldExtensionInstalled
1 parent 69b1cba commit ae68128

7 files changed

Lines changed: 61 additions & 34 deletions

File tree

src/configuration/conflict-detector.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,8 @@ export function detectConflicts(): ConflictInfo {
5656
const oldOrganizeOnSaveEnabled = oldOrganizeOnSave && ourOrganizeOnSave;
5757

5858
if (oldOrganizeOnSaveEnabled) {
59-
if (!conflicts.some(c => c.includes('Old TypeScript Hero extension'))) {
60-
conflicts.push('• Old TypeScript Hero "organizeOnSave" is enabled (will run on save alongside this extension)');
61-
}
59+
// Always add on-save conflict (separate from keyboard conflict — both can coexist)
60+
conflicts.push('• Old TypeScript Hero "organizeOnSave" is enabled (will run on save alongside this extension)');
6261
}
6362

6463
// Check 3: VS Code built-in organize imports enabled

src/configuration/settings-migration.ts

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -75,23 +75,29 @@ export async function migrateSettings(context: Pick<ExtensionContext, 'globalSta
7575
}
7676

7777
// Try to migrate settings
78-
const migratedCount = await performMigration();
79-
80-
// Mark migration as attempted (always, even if no settings found)
81-
await context.globalState.update(MIGRATION_KEY, true);
82-
83-
// Show notification if settings were migrated
84-
if (migratedCount > 0) {
85-
const oldExtension = extensions.getExtension(OLD_EXTENSION_ID);
86-
const isOldExtensionActive = oldExtension !== undefined;
87-
88-
if (isOldExtensionActive) {
89-
const message = `Mini TypeScript Hero: Migrated ${migratedCount} setting(s) from TypeScript Hero. You can now disable the old extension if you want.`;
90-
window.showInformationMessage(message);
91-
} else {
92-
const message = `Mini TypeScript Hero: Migrated ${migratedCount} setting(s) from TypeScript Hero.`;
93-
window.showInformationMessage(message);
78+
// CRITICAL: Always set migration flag in finally block to prevent retry loop
79+
// if any config.update() call fails during migration
80+
try {
81+
const migratedCount = await performMigration();
82+
83+
// Show notification if settings were migrated
84+
if (migratedCount > 0) {
85+
const oldExtension = extensions.getExtension(OLD_EXTENSION_ID);
86+
const isOldExtensionInstalled = oldExtension !== undefined;
87+
88+
if (isOldExtensionInstalled) {
89+
const message = `Mini TypeScript Hero: Migrated ${migratedCount} setting(s) from TypeScript Hero. You can now disable the old extension if you want.`;
90+
window.showInformationMessage(message);
91+
} else {
92+
const message = `Mini TypeScript Hero: Migrated ${migratedCount} setting(s) from TypeScript Hero.`;
93+
window.showInformationMessage(message);
94+
}
9495
}
96+
} catch {
97+
// Migration failed — swallow silently, finally block sets the flag to prevent retry loop
98+
} finally {
99+
// Mark migration as attempted (always, even if migration failed)
100+
await context.globalState.update(MIGRATION_KEY, true);
95101
}
96102
}
97103

@@ -165,10 +171,21 @@ async function performMigration(): Promise<number> {
165171
// - Blank line preservation (uses 'preserve' mode, keeps existing blank lines from source)
166172
// - Merge timing: When mergeImportsFromSameModule is true, merges BEFORE removeTrailingIndex (matches old bug)
167173
// - Type-only merging: Strips 'import type' keywords and allows merging type-only with value imports (old behavior)
174+
// Check if legacyMode is already set at any scope
175+
// NOTE: unscoped inspect() can't see workspaceFolderValue in multi-root workspaces,
176+
// so we must iterate each folder individually to detect folder-level settings
168177
const legacyModeInspect = newConfig.inspect('legacyMode');
178+
let folderLevelLegacyModeExists = false;
179+
for (const folder of workspace.workspaceFolders ?? []) {
180+
const scopedInspect = workspace.getConfiguration(NEW_SECTION, folder.uri).inspect('legacyMode');
181+
if (scopedInspect?.workspaceFolderValue !== undefined) {
182+
folderLevelLegacyModeExists = true;
183+
break;
184+
}
185+
}
169186
if (legacyModeInspect?.globalValue === undefined &&
170187
legacyModeInspect?.workspaceValue === undefined &&
171-
legacyModeInspect?.workspaceFolderValue === undefined) {
188+
!folderLevelLegacyModeExists) {
172189

173190
// Write legacyMode to the scopes where old settings were migrated
174191
if (migratedGlobalCount > 0) {

src/extension.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ async function checkForConflicts(
6161
).length;
6262

6363
const hasKeyboardConflict = conflicts.some(c => c.includes('Ctrl+Alt+O'));
64-
const hasVSCodeBuiltIn = vsCodeBuiltInEnabled;
64+
const hasVSCodeBuiltIn = vsCodeBuiltInEnabled && ourOrganizeOnSaveEnabled;
6565
const hasOldExtension = oldExtensionInstalled || oldOrganizeOnSaveEnabled;
6666

6767
if (conflicts.length === 1 && hasKeyboardConflict && !onSaveCount) {
@@ -102,8 +102,8 @@ async function checkForConflicts(
102102
// Determine which buttons to show based on conflict type
103103
const buttons: string[] = [];
104104

105-
// Can we auto-fix VSCode built-in?
106-
const canAutoFixVSCode = vsCodeBuiltInEnabled;
105+
// Can we auto-fix VSCode built-in? Only offer if it's an actual conflict (not just enabled)
106+
const canAutoFixVSCode = vsCodeBuiltInEnabled && ourOrganizeOnSaveEnabled;
107107

108108
if (canAutoFixVSCode) {
109109
buttons.push('Disable for Me');

src/imports/import-grouping/import-group-identifier-invalid-error.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
*/
44
export class ImportGroupIdentifierInvalidError extends Error {
55
constructor(identifier: string) {
6-
super();
7-
this.message = `The identifier "${identifier}" does not match a keyword or a regex pattern (/ .. /).`;
6+
super(`The identifier "${identifier}" does not match a keyword or a regex pattern (/ .. /).`);
87
}
98
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ export type ImportGroupSetting =
1414
| string
1515
| { identifier: ImportGroupKeyword | string; order: ImportGroupOrder };
1616

17-
const REGEX_REGEX_GROUP = /^\/.+\/[gimsuy]*$/;
17+
const REGEX_REGEX_GROUP = /^\/.+\/[dgimsuyv]*$/;
1818

1919
/**
2020
* Parser that takes the vscode - setting and creates import groups out of it.

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,16 @@ export class RegexImportGroup implements ImportGroup {
3535
) {
3636
// Compile regex once in constructor for performance
3737
// 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);
38+
try {
39+
const slashMatch = this.regex.match(/^\/(.+)\/([dgimsuyv]*)$/);
40+
if (slashMatch) {
41+
this.compiledRegex = new RegExp(slashMatch[1], slashMatch[2]);
42+
} else {
43+
this.compiledRegex = new RegExp(this.regex);
44+
}
45+
} catch {
46+
// Invalid regex pattern from user config — use a never-matching regex as fallback
47+
this.compiledRegex = /(?!)/;
4348
}
4449
}
4550

src/imports/import-manager.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -879,9 +879,12 @@ export class ImportManager {
879879
}
880880

881881
// Check for standalone comments
882+
// Match block comment continuation lines (* or */) but not code starting with * (e.g., generator calls)
883+
const isBlockCommentContinuation = trimmedText.startsWith('*') &&
884+
(trimmedText.length === 1 || trimmedText[1] === ' ' || trimmedText[1] === '/' || trimmedText[1] === '*');
882885
const isStandaloneComment = trimmedText.startsWith('//') ||
883886
trimmedText.startsWith('/*') ||
884-
trimmedText.startsWith('*');
887+
isBlockCommentContinuation;
885888

886889
if (isStandaloneComment) {
887890
commentsBetweenImports.push(lineText);
@@ -1159,7 +1162,9 @@ export class ImportManager {
11591162

11601163
const specifierStrings = imp.specifiers.map(spec => {
11611164
const baseSpec = spec.alias ? `${spec.specifier} as ${spec.alias}` : spec.specifier;
1162-
const typePrefix = spec.isTypeOnly ? 'type ' : '';
1165+
// In legacy mode, strip specifier-level type modifiers (old extension doesn't support them)
1166+
const isSpecTypeOnly = spec.isTypeOnly && !this.config.legacyMode(this.document.uri);
1167+
const typePrefix = isSpecTypeOnly ? 'type ' : '';
11631168
let result = `${typePrefix}${baseSpec}`;
11641169

11651170
// Preserve comments (in both modes — GOLDEN RULE: never delete user content)
@@ -1194,7 +1199,9 @@ export class ImportManager {
11941199
// e.g., "B, // end" not "B // end,"
11951200
const formattedSpecifiers = imp.specifiers.map((spec, index) => {
11961201
const baseSpec = spec.alias ? `${spec.specifier} as ${spec.alias}` : spec.specifier;
1197-
const typePrefix = spec.isTypeOnly ? 'type ' : '';
1202+
// In legacy mode, strip specifier-level type modifiers
1203+
const isSpecTypeOnly = spec.isTypeOnly && !this.config.legacyMode(this.document.uri);
1204+
const typePrefix = isSpecTypeOnly ? 'type ' : '';
11981205
let result = `${typePrefix}${baseSpec}`;
11991206

12001207
// Preserve comments with proper comma placement (GOLDEN RULE: never delete user content)

0 commit comments

Comments
 (0)