From 9cbb65f8f136c66ea009b7bf917dc566bc991a09 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Thu, 15 Dec 2022 01:25:17 -0500 Subject: [PATCH] Updates storage type-safety Updates & migrates `graph:hiddenRefs` to `graph:filters.excludeRefs` --- src/plus/webviews/graph/graphWebview.ts | 226 ++++++++++++++----------------- src/storage.ts | 228 ++++++++++---------------------- 2 files changed, 171 insertions(+), 283 deletions(-) diff --git a/src/plus/webviews/graph/graphWebview.ts b/src/plus/webviews/graph/graphWebview.ts index 6f45a23..c4d2841 100644 --- a/src/plus/webviews/graph/graphWebview.ts +++ b/src/plus/webviews/graph/graphWebview.ts @@ -51,7 +51,7 @@ import type { RepositoryChangeEvent, RepositoryFileSystemChangeEvent } from '../ import { Repository, RepositoryChange, RepositoryChangeComparisonMode } from '../../../git/models/repository'; import type { GitSearch } from '../../../git/search'; import { getSearchQueryComparisonKey } from '../../../git/search'; -import type { StoredGraphExcludedRef, StoredGraphFilters, StoredGraphIncludeOnlyRef } from '../../../storage'; +import type { StoredGraphFilters, StoredGraphIncludeOnlyRef } from '../../../storage'; import { executeActionCommand, executeCommand, executeCoreGitCommand, registerCommand } from '../../../system/command'; import { gate } from '../../../system/decorators/gate'; import { debug } from '../../../system/decorators/log'; @@ -91,7 +91,6 @@ import type { GraphExcludeTypes, GraphHostingServiceType, GraphIncludeOnlyRef, - GraphIncludeOnlyRefs, GraphMissingRefsMetadataType, GraphPullRequestMetadata, GraphRefMetadata, @@ -612,7 +611,7 @@ export class GraphWebview extends WebviewBase { } private onRefsVisibilityChanged(e: UpdateRefsVisibilityParams) { - this.updateExcludedRefs(e.refs, e.visible); + this.updateExcludedRefs(this._graph, e.refs, e.visible); } private onDoubleClickRef(e: DoubleClickedRefParams) { @@ -1286,88 +1285,56 @@ export class GraphWebview extends WebviewBase { private getExcludedTypes(graph: GitGraph | undefined): GraphExcludeTypes | undefined { if (graph == null) return undefined; - return this.getRepoFilters(graph)?.excludeTypes; + return this.getFiltersByRepo(graph)?.excludeTypes; } private getExcludedRefs(graph: GitGraph | undefined): Record | undefined { - // TODO: Migrate from graph:hiddenRefs to graph:filters[repoPath].excludeRefs - return this.filterExcludedRefs(this.container.storage.getWorkspace('graph:hiddenRefs'), graph); - } - - private getIncludeOnlyRefs(graph: GitGraph | undefined): Record | undefined { if (graph == null) return undefined; - const storedFilters = this.getRepoFilters(graph); - const storedIncludeOnlyRefs = storedFilters?.includeOnlyRefs; - if (storedIncludeOnlyRefs == null || Object.keys(storedIncludeOnlyRefs).length === 0) return undefined; + let filtersByRepo: Record | undefined; - const includeRemotes = !(storedFilters?.excludeTypes?.remotes ?? false); + const storedHiddenRefs = this.container.storage.getWorkspace('graph:hiddenRefs'); + if (storedHiddenRefs != null && Object.keys(storedHiddenRefs).length !== 0) { + // Migrate hidden refs to exclude refs + filtersByRepo = this.container.storage.getWorkspace('graph:filtersByRepo') ?? {}; - const includeOnlyRefs: Record = {}; + for (const id in storedHiddenRefs) { + const repoPath = getRepoPathFromBranchOrTagId(id); - for (const [key, value] of Object.entries(storedIncludeOnlyRefs)) { - let branch; - if (value.id === 'HEAD') { - branch = find(graph.branches.values(), b => b.current); - if (branch == null) continue; - - includeOnlyRefs[key] = { ...value, id: branch.id, name: branch.name }; - } else { - includeOnlyRefs[key] = value; + filtersByRepo[repoPath] = updateRecordValue( + filtersByRepo[repoPath]?.excludeRefs, + 'excludeRefs', + storedHiddenRefs[id], + ); } - // Add the upstream branches for any local branches if there are any and we aren't excluding them - if (includeRemotes && value.type === 'head') { - branch = branch ?? graph.branches.get(value.name); - if (branch?.upstream != null && !branch.upstream.missing) { - const id = getBranchId(graph.repoPath, true, branch.upstream.name); - includeOnlyRefs[id] = { - id: id, - type: 'remote', - name: getBranchNameWithoutRemote(branch.upstream.name), - owner: getRemoteNameFromBranchName(branch.upstream.name), - }; - } - } + void this.container.storage.storeWorkspace('graph:filtersByRepo', filtersByRepo); + void this.container.storage.deleteWorkspace('graph:hiddenRefs'); } - return includeOnlyRefs; - } - - private getRepoFilters(graph: GitGraph | undefined): StoredGraphFilters | undefined { - if (graph == null) return undefined; - - const filters = this.container.storage.getWorkspace('graph:filters'); - return filters?.[graph.repoPath]; - } - - private filterExcludedRefs( - excludeRefs: Record | undefined, - graph: GitGraph | undefined, - ): GraphExcludeRefs | undefined { - if (excludeRefs == null || graph == null) return undefined; + const storedExcludeRefs = (filtersByRepo?.[graph.repoPath] ?? this.getFiltersByRepo(graph))?.excludeRefs; + if (storedExcludeRefs == null || Object.keys(storedExcludeRefs).length === 0) return undefined; const useAvatars = configuration.get('graph.avatars', undefined, true); - const filteredRefs: GraphExcludeRefs = {}; - - for (const id in excludeRefs) { - if (getRepoPathFromBranchOrTagId(id) === graph.repoPath) { - const ref: GraphExcludedRef = { ...excludeRefs[id] }; - if (ref.type === 'remote' && ref.owner) { - const remote = graph.remotes.get(ref.owner); - if (remote != null) { - ref.avatarUrl = ( - (useAvatars ? remote.provider?.avatarUri : undefined) ?? - getRemoteIconUri(this.container, remote, this._panel!.webview.asWebviewUri.bind(this)) - )?.toString(true); - } + const excludeRefs: GraphExcludeRefs = {}; + + for (const id in storedExcludeRefs) { + const ref: GraphExcludedRef = { ...storedExcludeRefs[id] }; + if (ref.type === 'remote' && ref.owner) { + const remote = graph.remotes.get(ref.owner); + if (remote != null) { + ref.avatarUrl = ( + (useAvatars ? remote.provider?.avatarUri : undefined) ?? + getRemoteIconUri(this.container, remote, this._panel!.webview.asWebviewUri.bind(this)) + )?.toString(true); } - filteredRefs[id] = ref; } + + excludeRefs[id] = ref; } - return filteredRefs; + // For v13, we return directly the hidden refs without validating them // This validation has too much performance impact. So we decided to comment those lines // for v13 and have it as tech debt to solve after we launch. @@ -1379,57 +1346,73 @@ export class GraphWebview extends WebviewBase { // const [hiddenBranches, hiddenTags] = await Promise.all([ // this.repository.getBranches({ - // filter: b => !b.current && hiddenRefs[b.id] != undefined, + // filter: b => !b.current && excludeRefs[b.id] != undefined, // }), // this.repository.getTags({ - // filter: t => hiddenRefs[t.id] != undefined, + // filter: t => excludeRefs[t.id] != undefined, // }), // ]); // const filteredHiddenRefsById: GraphHiddenRefs = {}; // for (const hiddenBranch of hiddenBranches.values) { - // filteredHiddenRefsById[hiddenBranch.id] = hiddenRefs[hiddenBranch.id]; + // filteredHiddenRefsById[hiddenBranch.id] = excludeRefs[hiddenBranch.id]; // } // for (const hiddenTag of hiddenTags.values) { - // filteredHiddenRefsById[hiddenTag.id] = hiddenRefs[hiddenTag.id]; + // filteredHiddenRefsById[hiddenTag.id] = excludeRefs[hiddenTag.id]; // } // return filteredHiddenRefsById; - // For v13, we return directly the hidden refs without validating them - // return excludeRefs; + return excludeRefs; } - // TODO: Use this in getIncludeOnlyRefs once we are actually updating the value in storage (UX is hooked up) - private filterIncludeOnlyRefs( - includeOnlyRefs: Record | undefined, - graph: GitGraph | undefined, - ): GraphIncludeOnlyRefs | undefined { - if (includeOnlyRefs == null || graph == null) return undefined; + private getIncludeOnlyRefs(graph: GitGraph | undefined): Record | undefined { + if (graph == null) return undefined; - const useAvatars = configuration.get('graph.avatars', undefined, true); + const storedFilters = this.getFiltersByRepo(graph); + const storedIncludeOnlyRefs = storedFilters?.includeOnlyRefs; + if (storedIncludeOnlyRefs == null || Object.keys(storedIncludeOnlyRefs).length === 0) return undefined; - const filteredRefs: GraphIncludeOnlyRefs = {}; - - for (const id in includeOnlyRefs) { - if (getRepoPathFromBranchOrTagId(id) === graph.repoPath) { - const ref: GraphIncludeOnlyRef = { ...includeOnlyRefs[id] }; - if (ref.type === 'remote' && ref.owner) { - const remote = graph.remotes.get(ref.owner); - if (remote != null) { - ref.avatarUrl = ( - (useAvatars ? remote.provider?.avatarUri : undefined) ?? - getRemoteIconUri(this.container, remote, this._panel!.webview.asWebviewUri.bind(this)) - )?.toString(true); - } + const includeRemotes = !(storedFilters?.excludeTypes?.remotes ?? false); + + const includeOnlyRefs: Record = {}; + + for (const [key, value] of Object.entries(storedIncludeOnlyRefs)) { + let branch; + if (value.id === 'HEAD') { + branch = find(graph.branches.values(), b => b.current); + if (branch == null) continue; + + includeOnlyRefs[key] = { ...value, id: branch.id, name: branch.name }; + } else { + includeOnlyRefs[key] = value; + } + + // Add the upstream branches for any local branches if there are any and we aren't excluding them + if (includeRemotes && value.type === 'head') { + branch = branch ?? graph.branches.get(value.name); + if (branch?.upstream != null && !branch.upstream.missing) { + const id = getBranchId(graph.repoPath, true, branch.upstream.name); + includeOnlyRefs[id] = { + id: id, + type: 'remote', + name: getBranchNameWithoutRemote(branch.upstream.name), + owner: getRemoteNameFromBranchName(branch.upstream.name), + }; } - filteredRefs[id] = ref; } } - return filteredRefs; + return includeOnlyRefs; + } + + private getFiltersByRepo(graph: GitGraph | undefined): StoredGraphFilters | undefined { + if (graph == null) return undefined; + + const filters = this.container.storage.getWorkspace('graph:filtersByRepo'); + return filters?.[graph.repoPath]; } private getColumnSettings(columns: Record | undefined): GraphColumnsSettings { @@ -1613,72 +1596,64 @@ export class GraphWebview extends WebviewBase { void this.notifyDidChangeColumns(); } - private updateExcludedRefs(refs: GraphExcludedRef[], visible: boolean) { - let storedExcludedRefs = this.container.storage.getWorkspace('graph:hiddenRefs'); + private updateExcludedRefs(graph: GitGraph | undefined, refs: GraphExcludedRef[], visible: boolean) { + if (refs == null || refs.length === 0) return; + + let storedExcludeRefs: StoredGraphFilters['excludeRefs'] = this.getFiltersByRepo(graph)?.excludeRefs ?? {}; for (const ref of refs) { - storedExcludedRefs = updateRecordValue( - storedExcludedRefs, + storedExcludeRefs = updateRecordValue( + storedExcludeRefs, ref.id, visible ? undefined : { id: ref.id, type: ref.type, name: ref.name, owner: ref.owner }, ); } - void this.container.storage.storeWorkspace('graph:hiddenRefs', storedExcludedRefs); + void this.updateFiltersByRepo(graph, { excludeRefs: storedExcludeRefs }); void this.notifyDidChangeRefsVisibility(); } - private updateRepoFilters(graph: GitGraph | undefined, filters: StoredGraphFilters) { + private updateFiltersByRepo(graph: GitGraph | undefined, updates: Partial) { if (graph == null) throw new Error('Cannot save repository filters since Graph is undefined'); - const filtersByRepo = this.container.storage.getWorkspace('graph:filters'); + const filtersByRepo = this.container.storage.getWorkspace('graph:filtersByRepo'); return this.container.storage.storeWorkspace( - 'graph:filters', - updateRecordValue(filtersByRepo, graph.repoPath, filters), + 'graph:filtersByRepo', + updateRecordValue(filtersByRepo, graph.repoPath, { ...filtersByRepo?.[graph.repoPath], ...updates }), ); } private updateIncludeOnlyRefs(graph: GitGraph | undefined, refs: GraphIncludeOnlyRef[] | undefined) { - let storedFilters = this.getRepoFilters(graph); + let storedIncludeOnlyRefs: StoredGraphFilters['includeOnlyRefs']; + if (refs == null || refs.length === 0) { - if (storedFilters?.includeOnlyRefs == null) return; + if (this.getFiltersByRepo(graph)?.includeOnlyRefs == null) return; - storedFilters.includeOnlyRefs = undefined; + storedIncludeOnlyRefs = undefined; } else { - if (storedFilters == null) { - storedFilters = {}; - } - - let storedIncludeOnlyRefs = storedFilters.includeOnlyRefs ?? {}; + storedIncludeOnlyRefs = {}; for (const ref of refs) { - storedIncludeOnlyRefs = updateRecordValue(storedIncludeOnlyRefs, ref.id, { + storedIncludeOnlyRefs[ref.id] = { id: ref.id, type: ref.type, name: ref.name, owner: ref.owner, - }); + }; } - - storedFilters.includeOnlyRefs = storedIncludeOnlyRefs; } - void this.updateRepoFilters(graph, storedFilters); + void this.updateFiltersByRepo(graph, { includeOnlyRefs: storedIncludeOnlyRefs }); void this.notifyDidChangeRefsVisibility(); } private updateExcludedType(graph: GitGraph | undefined, { key, value }: UpdateExcludeTypeParams) { - let storedFilters = this.getRepoFilters(graph); - - const storedExcludedTypes = storedFilters?.excludeTypes; - if ((storedExcludedTypes == null || Object.keys(storedExcludedTypes).length === 0) && value === false) { + let excludeTypes = this.getFiltersByRepo(graph)?.excludeTypes; + if ((excludeTypes == null || Object.keys(excludeTypes).length === 0) && value === false) { return; } - if (storedFilters == null) { - storedFilters = {}; - } - storedFilters.excludeTypes = updateRecordValue(storedExcludedTypes, key, value); + excludeTypes = updateRecordValue(excludeTypes, key, value); - void this.updateRepoFilters(graph, storedFilters); + void this.updateFiltersByRepo(graph, { excludeTypes: excludeTypes }); void this.notifyDidChangeRefsVisibility(); } @@ -1976,6 +1951,7 @@ export class GraphWebview extends WebviewBase { if (refs != null) { this.updateExcludedRefs( + this._graph, refs.map(r => { const remoteBranch = r.refType === 'branch' && r.remote; return { diff --git a/src/storage.ts b/src/storage.ts index b2dddf7..ff34c7a 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -12,14 +12,14 @@ export type StorageChangeEvent = /** * The key of the stored value that has changed. */ - readonly key: GlobalStoragePath; + readonly key: keyof (GlobalStorage & DeprecatedGlobalStorage); readonly workspace: false; } | { /** * The key of the stored value that has changed. */ - readonly key: WorkspaceStoragePath; + readonly key: keyof (WorkspaceStorage & DeprecatedWorkspaceStorage); readonly workspace: true; }; @@ -43,27 +43,23 @@ export class Storage implements Disposable { this._disposable.dispose(); } - get(key: T): GlobalStoragePathValue; - get( - key: T, - defaultValue: NonNullable>, - ): NonNullable>; + get(key: T): GlobalStorage[T] | undefined; + /** @deprecated */ + get(key: T): DeprecatedGlobalStorage[T] | undefined; + get(key: T, defaultValue: GlobalStorage[T]): GlobalStorage[T]; @debug({ logThreshold: 50 }) - get( - key: T, - defaultValue?: GlobalStoragePathValue, - ): GlobalStoragePathValue | undefined { + get(key: keyof (GlobalStorage & DeprecatedGlobalStorage), defaultValue?: unknown): unknown | undefined { return this.context.globalState.get(`gitlens:${key}`, defaultValue); } @debug({ logThreshold: 250 }) - async delete(key: T): Promise { + async delete(key: keyof (GlobalStorage & DeprecatedGlobalStorage)): Promise { await this.context.globalState.update(`gitlens:${key}`, undefined); this._onDidChange.fire({ key: key, workspace: false }); } @debug({ args: { 1: false }, logThreshold: 250 }) - async store(key: T, value: GlobalStoragePathValue): Promise { + async store(key: T, value: GlobalStorage[T] | undefined): Promise { await this.context.globalState.update(`gitlens:${key}`, value); this._onDidChange.fire({ key: key, workspace: false }); } @@ -83,27 +79,26 @@ export class Storage implements Disposable { return this.context.secrets.store(key, value); } - getWorkspace(key: T): WorkspaceStoragePathValue; - getWorkspace( - key: T, - defaultValue: NonNullable>, - ): NonNullable>; + getWorkspace(key: T): WorkspaceStorage[T] | undefined; + /** @deprecated */ + getWorkspace(key: T): DeprecatedWorkspaceStorage[T] | undefined; + getWorkspace(key: T, defaultValue: WorkspaceStorage[T]): WorkspaceStorage[T]; @debug({ logThreshold: 25 }) - getWorkspace( - key: T, - defaultValue?: WorkspaceStoragePathValue, - ): WorkspaceStoragePathValue | undefined { + getWorkspace( + key: keyof (WorkspaceStorage & DeprecatedWorkspaceStorage), + defaultValue?: unknown, + ): unknown | undefined { return this.context.workspaceState.get(`gitlens:${key}`, defaultValue); } @debug({ logThreshold: 250 }) - async deleteWorkspace(key: T): Promise { + async deleteWorkspace(key: keyof (WorkspaceStorage & DeprecatedWorkspaceStorage)): Promise { await this.context.workspaceState.update(`gitlens:${key}`, undefined); this._onDidChange.fire({ key: key, workspace: true }); } @debug({ args: { 1: false }, logThreshold: 250 }) - async storeWorkspace(key: T, value: WorkspaceStoragePathValue): Promise { + async storeWorkspace(key: keyof WorkspaceStorage, value: unknown | undefined): Promise { await this.context.workspaceState.update(`gitlens:${key}`, value); this._onDidChange.fire({ key: key, workspace: true }); } @@ -111,117 +106,65 @@ export class Storage implements Disposable { export type SecretKeys = string; -export const enum DeprecatedStorageKeys { - /** @deprecated */ - DisallowConnectionPrefix = 'gitlens:disallow:connection:', -} - export const enum SyncedStorageKeys { Version = 'gitlens:synced:version', PreReleaseVersion = 'gitlens:synced:preVersion', HomeViewWelcomeVisible = 'gitlens:views:welcome:visible', } -export interface GlobalStorage { - avatars?: [string, StoredAvatar][]; - provider: { - authentication: { - skip: Record; - }; - }; - home: { - actions: { - completed?: CompletedActions[]; - }; - steps: { - completed?: string[]; - }; - sections: { - dismissed?: string[]; - }; - status: { - pinned?: boolean; - }; - banners: { - dismissed?: string[]; - }; - }; - pendingWelcomeOnFocus?: boolean; - pendingWhatsNewOnFocus?: boolean; - plus: { - migratedAuthentication?: boolean; - discountNotificationShown?: boolean; - }; +export type DeprecatedGlobalStorage = { + /** @deprecated */ + [key in `disallow:connection:${string}`]: any; +}; + +export type GlobalStorage = { + avatars: [string, StoredAvatar][]; + 'home:actions:completed': CompletedActions[]; + 'home:steps:completed': string[]; + 'home:sections:dismissed': string[]; + 'home:status:pinned': boolean; + 'home:banners:dismissed': string[]; + pendingWelcomeOnFocus: boolean; + pendingWhatsNewOnFocus: boolean; + 'plus:migratedAuthentication': boolean; + 'plus:discountNotificationShown': boolean; // Don't change this key name ('premium`) as its the stored subscription - premium: { - subscription?: Stored; - }; - synced: { - version?: string; - // Keep the pre-release version separate from the released version - preVersion?: string; - }; - usages?: Record; - version?: string; + 'premium:subscription': Stored; + 'synced:version': string; // Keep the pre-release version separate from the released version - preVersion?: string; - views: { - layout?: StoredViewsLayout; - welcome: { - visible?: boolean; - }; - commitDetails: { - dismissed?: string[]; - }; - }; -} - -export interface WorkspaceStorage { + 'synced:preVersion': string; + usages: Record; + version: string; + // Keep the pre-release version separate from the released version + preVersion: string; + 'views:layout': StoredViewsLayout; + 'views:welcome:visible': boolean; + 'views:commitDetails:dismissed': string[]; +} & { [key in `provider:authentication:skip:${string}`]: boolean }; + +export type DeprecatedWorkspaceStorage = { + /** @deprecated use `graph:filtersByRepo.excludeRefs` */ + 'graph:hiddenRefs': Record; + /** @deprecated use `views:searchAndCompare:pinned` */ + 'pinned:comparisons': Record; +}; + +export type WorkspaceStorage = { assumeRepositoriesOnStartup?: boolean; - branch: { - comparisons?: StoredBranchComparisons; - }; - connected: Record; - gitComandPalette: { - usage?: RecentUsage; - }; - gitPath?: string; - graph: { - banners: { - dismissed?: Record; - }; - columns?: Record; - // this should be excludeRefs, but we need to keep this name for backward compatibility - hiddenRefs?: Record; - includeOnlyRefs?: Record; - excludeTypes?: Record; - filters: Record; - }; - remote: { - default?: string; - }; - starred: { - branches?: StoredStarred; - repositories?: StoredStarred; - }; - views: { - repositories: { - autoRefresh?: boolean; - }; - searchAndCompare: { - keepResults?: boolean; - pinned?: StoredPinnedItems; - }; - commitDetails: { - autolinksExpanded?: boolean; - }; - }; - - pinned: { - /** @deprecated use `gitlens:views:searchAndCompare:pinned` */ - comparisons?: DeprecatedPinnedComparisons; - }; -} + 'branch:comparisons': StoredBranchComparisons; + 'gitComandPalette:usage': RecentUsage; + gitPath: string; + 'graph:banners:dismissed': Record; + 'graph:columns': Record; + 'graph:filtersByRepo': Record; + 'remote:default': string; + 'starred:branches': StoredStarred; + 'starred:repositories': StoredStarred; + 'views:repositories:autoRefresh': boolean; + 'views:searchAndCompare:keepResults': boolean; + 'views:searchAndCompare:pinned': StoredPinnedItems; + 'views:commitDetails:autolinksExpanded': boolean; +} & { [key in `connected:${string}`]: boolean }; export type StoredViewsLayout = 'gitlens' | 'scm'; export interface Stored { @@ -251,9 +194,8 @@ export interface StoredGraphColumn { export interface StoredGraphFilters { includeOnlyRefs?: Record; - excludeTypes?: Record; - // TODO migrate data here from hiddenRefs in the root graph storage excludeRefs?: Record; + excludeTypes?: Record; } export type StoredGraphRefType = 'head' | 'remote' | 'tag'; @@ -313,33 +255,3 @@ interface DeprecatedPinnedComparison { ref2: StoredNamedRef; notation?: '..' | '...'; } - -interface DeprecatedPinnedComparisons { - [id: string]: DeprecatedPinnedComparison; -} - -type SubPath = Key extends string - ? T[Key] extends Record - ? - | `${Key}:${SubPath>}` - | `${Key}:${Exclude & string}` - : never - : never; - -type Path = SubPath | keyof T; - -type PathValue> = P extends `${infer Key}:${infer Rest}` - ? Key extends keyof T - ? Rest extends Path - ? PathValue - : never - : never - : P extends keyof T - ? T[P] - : never; - -type GlobalStoragePath = Path; -type GlobalStoragePathValue

= PathValue; - -type WorkspaceStoragePath = Path; -type WorkspaceStoragePathValue

= PathValue;