Source Type Migration Reconciliation - #248
Conversation
1301b46 to
81ca4e1
Compare
81ca4e1 to
23bc366
Compare
| * Avoids circular dependency by depending on the interface, not the class. | ||
| */ | ||
| export interface ReconcilerRegistryOperations { | ||
| uninstallBundle(bundleId: string, scope: InstallationScope, silent?: boolean): Promise<void>; |
There was a problem hiding this comment.
[format personal preferences]
| uninstallBundle(bundleId: string, scope: InstallationScope, silent?: boolean): Promise<void>; | |
| uninstallBundle: (bundleId: string, scope: InstallationScope, silent?: boolean) => Promise<void>; |
| return existingSources.find((existing) => { | ||
| if (existing.type !== 'awesome-copilot') { | ||
| return false; | ||
| } | ||
| const existingUrl = normalizeUrl(existing.url); | ||
| return existingUrl === newUrl; | ||
| }); |
There was a problem hiding this comment.
| return existingSources.find((existing) => { | |
| if (existing.type !== 'awesome-copilot') { | |
| return false; | |
| } | |
| const existingUrl = normalizeUrl(existing.url); | |
| return existingUrl === newUrl; | |
| }); | |
| return existingSources.find((existing) => existing.type === 'awesome-copilot' | |
| ? normalizeUrl(existing.url) === newUrl | |
| : false; | |
| }); |
notation could be simplify
| const all = await this.registry.listInstalledBundles(); | ||
| const seen = new Set<string>(); | ||
| return all.filter((bundle) => { | ||
| const key = `${bundle.bundleId}:${bundle.scope}`; | ||
| if (seen.has(key)) { | ||
| return false; | ||
| } | ||
| seen.add(key); | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
| const all = await this.registry.listInstalledBundles(); | |
| const seen = new Set<string>(); | |
| return all.filter((bundle) => { | |
| const key = `${bundle.bundleId}:${bundle.scope}`; | |
| if (seen.has(key)) { | |
| return false; | |
| } | |
| seen.add(key); | |
| return true; | |
| }); | |
| return Array.from(new Set(await this.registry.listInstalledBundles())); |
| }); | ||
| } | ||
|
|
||
| private async reconcileBundle( |
There was a problem hiding this comment.
The code could be simplified and splitted
private async isBundleInstallable(newBundleId: string, oldBundleId: string) {
try {
await this.registry.getBundleDetails(newBundleId);
return true;
} catch (error) {
this.logger.warn(
`[SourceTypeReconciler] New github bundle '${newBundleId}' is not available. `
+ `Keeping old installation for '${oldBundleId}'. Error: ${(error as Error).message}`
);
return false;
}
}
private async reconcileBundle(
oldBundleId: string,
newBundle: Bundle,
oldSourceId: string,
allInstalled: InstalledBundle[]
): Promise<BundleReconciliationResult[]> {
const scopeInstalls = allInstalled.filter(
(b) => b.bundleId === oldBundleId && b.sourceId === oldSourceId
);
if (scopeInstalls.length === 0) {
this.logger.debug(
`[SourceTypeReconciler] Bundle '${oldBundleId}' not found in any scope. Skipping.`
);
return [];
}
const installable = await isBundleInstallable(newBundle.id, oldBundleId);
if (!installable) {
return scopeInstalls.map((scopeInstall) => ({
oldBundleId,
newBundleId: newBundle.id,
scope: scopeInstall.scope,
success: false,
error: 'New github bundle not available'
}));
}
return Promise.all(scopeInstalls.map(async (scopeInstall) => {
const existingTargetInstall = allInstalled.find(
(b) => b.bundleId === newBundle.id && b.scope === scopeInstall.scope
);
try {
if (existingTargetInstall) {
this.logger.info(
`[SourceTypeReconciler] Bundle '${newBundle.id}' already installed in scope `
+ `'${scopeInstall.scope}'. Uninstalling old bundle '${oldBundleId}' without reinstall.`
);
await this.registry.uninstallBundle(oldBundleId, scopeInstall.scope, true);
return {
oldBundleId,
newBundleId: newBundle.id,
scope: scopeInstall.scope,
success: true
};
}
this.logger.info(
`[SourceTypeReconciler] Migrating '${oldBundleId}' → '${newBundle.id}' in scope '${scopeInstall.scope}'`
);
// Install new bundle first (different IDs so they coexist),
// then uninstall old — ensures we never leave the user without either.
await this.registry.installBundle(
newBundle.id,
{
scope: scopeInstall.scope,
force: true,
profileId: scopeInstall.profileId
},
true
);
await this.registry.uninstallBundle(oldBundleId, scopeInstall.scope, true);
return {
oldBundleId,
newBundleId: newBundle.id,
scope: scopeInstall.scope,
success: true
};
} catch (error) {
const errorMsg = (error as Error).message;
this.logger.error(
`[SourceTypeReconciler] Failed to migrate '${oldBundleId}' in scope '${scopeInstall.scope}': ${errorMsg}`
);
return {
oldBundleId,
newBundleId: newBundle.id,
scope: scopeInstall.scope,
success: false,
error: errorMsg
};
}
}));
}| } | ||
|
|
||
| return updatedCount; | ||
| } |
There was a problem hiding this comment.
The let changed + mutation pattern makes this harder to follow than needed. Consider extracting a pure helper that returns the updated state or undefined if nothing changed — removes the let and side-effect assignments inside .map().
Also, let updatedCount can be avoided by filtering first and counting settled results. Using Promise.allSettled parallelizes the saves and ensures one failure does not block the others.
| } | |
| private buildUpdatedProfileState( | |
| profile: ProfileActivationState, | |
| mapping: Map<string, Bundle> | |
| ): ProfileActivationState | undefined { | |
| const newSyncedBundles = profile.syncedBundles.map((id) => mapping.get(id)?.id ?? id); | |
| const newVersions: Record<string, string> = Object.fromEntries( | |
| Object.entries(profile.syncedBundleVersions ?? {}).map(([id, version]) => { | |
| const newBundle = mapping.get(id); | |
| return newBundle ? [newBundle.id, newBundle.version] : [id, version]; | |
| }) | |
| ); | |
| const changed = newSyncedBundles.some((id, i) => id !== profile.syncedBundles[i]) | |
| || Object.keys(newVersions).some((k) => !Object.keys(profile.syncedBundleVersions ?? {}).includes(k)); | |
| return changed ? { ...profile, syncedBundles: newSyncedBundles, syncedBundleVersions: newVersions } : undefined; | |
| } | |
| private async updateProfileActivationStates( | |
| hubId: string, | |
| mapping: Map<string, Bundle> | |
| ): Promise<number> { | |
| try { | |
| const activeProfiles = await this.hubStorage.listActiveProfiles(); | |
| const hubProfiles = activeProfiles.filter((p) => p.hubId === hubId); | |
| const profilesToUpdate = hubProfiles | |
| .map((profile) => ({ profile, updatedState: this.buildUpdatedProfileState(profile, mapping) })) | |
| .filter((entry) => entry.updatedState !== undefined); | |
| const results = await Promise.allSettled( | |
| profilesToUpdate.map(async ({ profile, updatedState }) => { | |
| await this.hubStorage.saveProfileActivationState(hubId, profile.profileId, updatedState!); | |
| this.logger.info( | |
| `[SourceTypeReconciler] Updated profile activation state: ${profile.profileId}` | |
| ); | |
| }) | |
| ); | |
| return results.filter((r) => r.status === 'fulfilled').length; | |
| } catch (error) { | |
| this.logger.warn( | |
| `[SourceTypeReconciler] Failed to update profile activation states: ${(error as Error).message}` | |
| ); | |
| return 0; | |
| } | |
| } |
|
|
||
| for (const [oldBundleId, newBundle] of mapping) { | ||
| const scopeResults = await this.reconcileBundle(oldBundleId, newBundle, oldSource.id, allInstalled); | ||
| result.bundleResults.push(...scopeResults); |
There was a problem hiding this comment.
Bundle reconciliations are independent of each other — they can run in parallel with Promise.allSettled for better performance and resilience (one failure does not block the rest).
| result.bundleResults.push(...scopeResults); | |
| const allResults = await Promise.allSettled( | |
| [...mapping].map(async ([oldBundleId, newBundle]) => | |
| this.reconcileBundle(oldBundleId, newBundle, oldSource.id, allInstalled) | |
| ) | |
| ); | |
| result.bundleResults = allResults | |
| .filter((r) => r.status === 'fulfilled') | |
| .flatMap((r) => r.value); |
| ); | ||
| const successfulMapping = new Map( | ||
| [...mapping].filter(([oldId]) => migratedIds.has(oldId)) | ||
| ); |
There was a problem hiding this comment.
The intermediate resultsByBundle grouping is not needed — a bundle counts as "migrated" if any of its results succeeded, which is equivalent to checking if its oldBundleId appears in the set of successful results. This removes the mutable Map + imperative loop.
| ); | |
| const migratedIds = new Set( | |
| result.bundleResults | |
| .filter((r) => r.success) | |
| .map((r) => r.oldBundleId) | |
| ); | |
| const successfulMapping = new Map( | |
| [...mapping].filter(([oldId]) => migratedIds.has(oldId)) | |
| ); |
| } | ||
|
|
||
| return mapping; | ||
| } |
There was a problem hiding this comment.
Same pattern: mutable Map + imperative loop with continue can be replaced by a functional .map() + .filter() pipeline, returning a new Map() from the entries.
| } | |
| public static buildBundleIdMapping( | |
| installedBundles: InstalledBundle[], | |
| githubBundles: Bundle[], | |
| oldSourceId: string, | |
| githubSource?: RegistrySource | |
| ): Map<string, Bundle> { | |
| const logger = Logger.getInstance(); | |
| const sourceMetadata = githubSource?.url | |
| ? extractGitHubMetadata(githubSource.url) | |
| : undefined; | |
| const oldBundles = installedBundles.filter( | |
| (b) => b.sourceId === oldSourceId | |
| ); | |
| const entries = oldBundles | |
| .map((oldBundle) => { | |
| const matches = githubBundles.filter((gb) => | |
| BundleIdentityMatcher.matchesAwesomeCopilotToGithub( | |
| oldBundle.bundleId, | |
| gb.id, | |
| sourceMetadata | |
| ) | |
| ); | |
| if (matches.length === 0) { | |
| logger.warn( | |
| `[SourceTypeReconciler] No github bundle matches '${oldBundle.bundleId}'. Skipping.` | |
| ); | |
| return undefined; | |
| } | |
| const latest = matches.reduce((best, candidate) => | |
| VersionManager.compareVersions(candidate.version, best.version) > 0 | |
| ? candidate | |
| : best | |
| ); | |
| if (matches.length === 1) { | |
| logger.info( | |
| `[SourceTypeReconciler] Mapped: ${oldBundle.bundleId} → ${latest.id}` | |
| ); | |
| } else { | |
| logger.info( | |
| `[SourceTypeReconciler] Mapped: ${oldBundle.bundleId} → ${latest.id} ` | |
| + `(selected latest of ${matches.length} candidates)` | |
| ); | |
| } | |
| return [oldBundle.bundleId, latest] as const; | |
| }) | |
| .filter((entry): entry is [string, Bundle] => entry !== undefined); | |
| return new Map(entries); | |
| } |
| @@ -163,7 +163,6 @@ export class VersionManager { | |||
|
|
|||
| if (match && match.index !== undefined) { | |||
| const identity = bundleId.slice(0, match.index); | |||
| this.logger.debug(`Extracted bundle identity: "${bundleId}" -> "${identity}"`); | |||
| return identity; | |||
| } | |||
There was a problem hiding this comment.
Unnecessary intermediate variable.
| } | |
| if (match && match.index !== undefined) { | |
| return bundleId.slice(0, match.index); | |
| } |
| export function extractGitHubMetadata(url: string): { owner: string; repo: string } | undefined { | ||
| const match = url.match(/github\.com\/([^/]+)\/([^/]+)/i); | ||
| if (!match) { | ||
| return undefined; | ||
| } | ||
| const repo = match[2].replace(/\.git$/, ''); | ||
| return { | ||
| owner: match[1], | ||
| repo | ||
| }; | ||
| } |
There was a problem hiding this comment.
The regex already captures owner and repo — we can destructure directly and handle the .git suffix in the regex itself.
| export function extractGitHubMetadata(url: string): { owner: string; repo: string } | undefined { | |
| const match = url.match(/github\.com\/([^/]+)\/([^/]+)/i); | |
| if (!match) { | |
| return undefined; | |
| } | |
| const repo = match[2].replace(/\.git$/, ''); | |
| return { | |
| owner: match[1], | |
| repo | |
| }; | |
| } | |
| export function extractGitHubMetadata(url: string): { owner: string; repo: string } | undefined { | |
| const match = url.match(/github\.com\/([^/]+)\/([^/.]+)(?:\.git)?/i); | |
| if (!match) { | |
| return undefined; | |
| } | |
| const [, owner, repo] = match; | |
| return { owner, repo }; | |
| } |
There was a problem hiding this comment.
Done with a small tweak used ([^/]+?) instead of ([^/.]+) to support repo with a dot in the name such as genai.prompt-registry-config
23bc366 to
da2e061
Compare
fix: resolve lint errors in source type migration files
14b7302 to
49c3ded
Compare
fix: resolve lint errors in source type migration files
Description
Automatically detect and handle hub source type changes (e.g., awesome-copilot → github) during hub sync. Maps old bundle IDs to new ones, uninstalls bundles from the old source, and reinstalls from the new source without breaking existing installations.
Type of Change
Related Issues
Relates to bundle reconciliation during hub migrations
Changes Made
SourceTypeReconcilerservice to orchestrate source type migration (detect changes, build ID mappings, uninstall/reinstall bundles)matchesCrossType()toBundleIdentityMatcherfor awesome-copilot ↔ github ID matchingHubManager.loadHubSources()to detect source type changes and trigger reconciliationRegistryManagerfor bundle matching across source type changesTesting
Test Coverage
Implementation Approach
Used test-driven development (TDD) to implement each component: