From a4e88cf2c7940c8a67baaa460949d3e394e6341a Mon Sep 17 00:00:00 2001 From: John Harrington <84741727+harr1424@users.noreply.github.com> Date: Tue, 19 Aug 2025 06:05:28 -0700 Subject: [PATCH] WIP: adding errors and warnings to UI --- .../de-duplicate/de-duplicate.component.html | 26 +++- .../de-duplicate/de-duplicate.component.ts | 84 ++++++++++- .../vault/services/de-duplicate.service.ts | 140 +++++++++++++++--- apps/web/src/locales/en/messages.json | 74 +++++++++ 4 files changed, 305 insertions(+), 19 deletions(-) diff --git a/apps/web/src/app/tools/de-duplicate/de-duplicate.component.html b/apps/web/src/app/tools/de-duplicate/de-duplicate.component.html index e9fac19255a..c3b38a65882 100644 --- a/apps/web/src/app/tools/de-duplicate/de-duplicate.component.html +++ b/apps/web/src/app/tools/de-duplicate/de-duplicate.component.html @@ -11,10 +11,34 @@ {{ callout.message }} + +
+
{{ warningsCallout.message }}
+ +
+ +
{{ "uriMatchingStrategy" | i18n }} - + {{ "uriMatchingStrategyHelp" | i18n }} [] = [ @@ -52,10 +65,12 @@ export class DeDuplicateComponent { private i18nService: I18nService, private accountService: AccountService, private cdr: ChangeDetectorRef, + private dialogService: DialogService, ) {} findDuplicates = async () => { this.callout = null; + this.warningsCallout = null; // Allow progress spinner to appear on button await new Promise((r) => setTimeout(r, 100)); @@ -84,6 +99,45 @@ export class DeDuplicateComponent { message: parts.join(", ") + ".", }; } + + // Summarize warnings if any + const w = result.warnings; + if (w && (w.exactFallbackCount || w.unparseableUriCount || w.permissionDeniedCount)) { + const warningParts: string[] = []; + if (w.exactFallbackCount) { + const key = + w.exactFallbackCount === 1 ? "duplicateExactFallback" : "duplicateExactFallbackPlural"; + warningParts.push(this.i18nService.t(key, w.exactFallbackCount)); + } + if (w.unparseableUriCount) { + const key = + w.unparseableUriCount === 1 + ? "duplicateUnparseableUris" + : "duplicateUnparseableUrisPlural"; + warningParts.push(this.i18nService.t(key, w.unparseableUriCount)); + } + if (w.permissionDeniedCount) { + const key = + w.permissionDeniedCount === 1 + ? "duplicatePermissionDenied" + : "duplicatePermissionDeniedPlural"; + warningParts.push(this.i18nService.t(key, w.permissionDeniedCount)); + } + const title = this.i18nService.t("duplicateWarningsTitle"); + this.warnings = w; + this.warningsCallout = { + type: "warning", + title, + message: warningParts.join(" \u2022 "), + detailsButtonText: + (w.unparseableUris?.length || 0) > 0 || + (w.exactFallbackSamples?.length || 0) > 0 || + (w.permissionDeniedNames?.length || 0) > 0 + ? this.i18nService.t("duplicateWarningsDetailsButton") + : undefined, + details: w.unparseableUris, + }; + } } catch (e) { const message = this.i18nService.t("duplicateError"); this.callout = { @@ -95,4 +149,32 @@ export class DeDuplicateComponent { this.cdr.markForCheck?.(); } }; + + async openWarningsDetails() { + const w = this.warnings; + const bodyLines: string[] = []; + if (w?.unparseableUris?.length) { + bodyLines.push(this.i18nService.t("duplicateWarningsDialogUnparseableTitle")); + bodyLines.push(...w.unparseableUris.slice(0, 10).map((d) => `• ${d}`)); + bodyLines.push(""); + } + if (w?.exactFallbackSamples?.length) { + bodyLines.push(this.i18nService.t("duplicateWarningsDialogExactSamplesTitle")); + bodyLines.push(...w.exactFallbackSamples.slice(0, 10).map((d) => `• ${d}`)); + bodyLines.push(""); + } + if (w?.permissionDeniedNames?.length) { + bodyLines.push(this.i18nService.t("duplicateWarningsDialogPermissionDeniedTitle")); + bodyLines.push(...w.permissionDeniedNames.slice(0, 10).map((n) => `• ${n}`)); + bodyLines.push(""); + } + bodyLines.push(this.i18nService.t("duplicateWarningsDialogExactHelp")); + bodyLines.push(this.i18nService.t("duplicateWarningsDialogPermHelp")); + await this.dialogService.openSimpleDialog({ + title: this.i18nService.t("duplicateWarningsDialogTitle"), + content: bodyLines.join("\n"), + type: "info", + acceptButtonText: { key: "ok" }, + }); + } } diff --git a/apps/web/src/app/vault/services/de-duplicate.service.ts b/apps/web/src/app/vault/services/de-duplicate.service.ts index 0e954a36665..af8d85fe7a1 100644 --- a/apps/web/src/app/vault/services/de-duplicate.service.ts +++ b/apps/web/src/app/vault/services/de-duplicate.service.ts @@ -17,6 +17,16 @@ export interface DuplicateOperationResult { setsFound: number; trashed: number; permanentlyDeleted: number; + warnings?: DuplicateOperationWarnings; +} + +export interface DuplicateOperationWarnings { + exactFallbackCount: number; + unparseableUriCount: number; + permissionDeniedCount: number; + unparseableUris?: string[]; + exactFallbackSamples?: string[]; + permissionDeniedNames?: string[]; } interface DuplicateSet { @@ -35,6 +45,10 @@ interface ParsedUri { path?: string; query?: string; fragment?: string; + /** True when URL parsing fell back to a best-effort/manual parse (not fully exact). */ + approximate?: boolean; + /** True when the input could not be parsed into a host/authority at all. */ + unparseable?: boolean; } @Injectable({ @@ -68,13 +82,28 @@ export class DeDuplicateService { ): Promise { const uriStrategy = options?.uriStrategy ?? DeDuplicateService.DEFAULT_URI_STRATEGY; const allCiphers = await this.cipherService.getAllDecrypted(userId); - const duplicateSets = this.findDuplicateSets(allCiphers, uriStrategy); + const warningAccumulator = this.createWarningAccumulator(); + const duplicateSets = this.findDuplicateSets(allCiphers, uriStrategy, warningAccumulator); if (duplicateSets.length > 0) { - const { trashed, permanentlyDeleted } = await this.handleDuplicates(duplicateSets, userId); - return { setsFound: duplicateSets.length, trashed, permanentlyDeleted }; + const { trashed, permanentlyDeleted } = await this.handleDuplicates( + duplicateSets, + userId, + warningAccumulator, + ); + return { + setsFound: duplicateSets.length, + trashed, + permanentlyDeleted, + warnings: this.toWarningsResult(warningAccumulator), + }; } - return { setsFound: 0, trashed: 0, permanentlyDeleted: 0 }; + return { + setsFound: 0, + trashed: 0, + permanentlyDeleted: 0, + warnings: this.toWarningsResult(warningAccumulator), + }; } /** @@ -90,6 +119,7 @@ export class DeDuplicateService { private findDuplicateSets( ciphers: CipherView[], uriStrategy: UriMatchStrategy = DeDuplicateService.DEFAULT_URI_STRATEGY, + warningAccumulator?: WarningAccumulator, ): DuplicateSet[] { const uriBuckets = new Map(); const nameBuckets = new Map(); @@ -127,7 +157,7 @@ export class DeDuplicateService { const uris = this.extractUriStrings(cipher); if (uris.length > 0) { // Collect unique normalized keys to avoid adding the same cipher twice to the same bucket - const keys = this.getUriKeysForStrategy(uris, uriStrategy); + const keys = this.getUriKeysForStrategy(uris, uriStrategy, warningAccumulator); for (const k of keys) { const key = `${username}||${k}`; let bucket = uriBuckets.get(key); @@ -239,13 +269,25 @@ export class DeDuplicateService { /** * Get normalized keys for the given URIs according to a strategy. */ - private getUriKeysForStrategy(uris: string[], strategy: UriMatchStrategy): Set { + private getUriKeysForStrategy( + uris: string[], + strategy: UriMatchStrategy, + warningAccumulator?: WarningAccumulator, + ): Set { const keys = new Set(); for (const raw of uris) { const parsed = this.parseUri(raw); if (!parsed) { continue; } + // Count unparseable URIs and skip them + if (parsed.unparseable) { + if (warningAccumulator) { + warningAccumulator.unparseableUriCount++; + warningAccumulator.unparseableUris.push(parsed.original); + } + continue; + } switch (strategy) { case "Base": { const base = this.getBaseUri(parsed); @@ -269,6 +311,12 @@ export class DeDuplicateService { break; } case "Exact": { + if (parsed.approximate && warningAccumulator) { + warningAccumulator.exactFallbackCount++; + if (warningAccumulator.exactFallbackSamples.length < 10) { + warningAccumulator.exactFallbackSamples.push(parsed.original); + } + } const exact = this.getExactUrlKey(parsed); if (exact) { keys.add(exact); @@ -334,12 +382,13 @@ export class DeDuplicateService { fragment, }; } catch { - // Fallback manual authority extraction sufficient for host, hostname, and base strategies - // TODO if exact strategy is being used, the user should be notified URI matching won't be exact + // Fallback manual authority extraction sufficient for Hostname/Base/Host strategies. + // We mark the result as `approximate`; getUriKeysForStrategy counts a fallback warning + // only when the selected strategy is "Exact". Other strategies continue normally. const authorityMatch = toParse.match(/^[a-z][a-z0-9+.-]*:\/\/([^/?#]+)/i); if (!authorityMatch) { - // TODO the user should be informed that this URI couldn't be matched - return { original: input }; + // Mark as unparseable so the caller can report and skip safely + return { original: input, unparseable: true }; } let authority = authorityMatch[1]; const atIndex = authority.lastIndexOf("@"); @@ -375,7 +424,7 @@ export class DeDuplicateService { } } host = host.replace(/\.$/, "").toLowerCase(); - return { original: input, hostname: host, port }; + return { original: input, hostname: host, port, approximate: true }; } } @@ -465,6 +514,7 @@ export class DeDuplicateService { private async handleDuplicates( duplicateSets: DuplicateSet[], userId: UserId, + warningAccumulator?: WarningAccumulator, ): Promise<{ trashed: number; permanentlyDeleted: number }> { // 1. Open the dialog to let the user review and select duplicates to delete. const dialogRef = DuplicateReviewDialogComponent.open(this.dialogService, { @@ -488,20 +538,40 @@ export class DeDuplicateService { } // 3. Filter the user's selected deletions based on their permissions. - // TODO: Determine why a user may not have permission to delete a cipher and explore ways of notifying them of this situation + // When users lack permission to delete an item (e.g., org-owned or restricted by collection permissions), + // record this so the UI can notify them. const permissionChecks = uniqueDeleteIds.map(async (id) => { const cipher = cipherIndex.get(id); if (!cipher) { - return null; + return { cipher: null as CipherView | null, canDelete: false }; } const canDelete = await firstValueFrom( this.cipherAuthorizationService.canDeleteCipher$(cipher), ); - return canDelete ? cipher : null; + return { cipher, canDelete }; }); - const permitted = (await Promise.all(permissionChecks)).filter( - (c): c is CipherView => c != null, - ); + const permissionResults = await Promise.all(permissionChecks); + const permitted: CipherView[] = []; + const denied: CipherView[] = []; + for (const r of permissionResults) { + if (!r.cipher) { + continue; + } + if (r.canDelete) { + permitted.push(r.cipher); + } else { + denied.push(r.cipher); + } + } + if (warningAccumulator && denied.length > 0) { + warningAccumulator.permissionDeniedCount += denied.length; + for (const d of denied) { + const name = d.name?.trim(); + if (name && warningAccumulator.permissionDeniedNames.length < 10) { + warningAccumulator.permissionDeniedNames.push(name); + } + } + } if (permitted.length === 0) { return { trashed: 0, permanentlyDeleted: 0 }; } @@ -545,4 +615,40 @@ export class DeDuplicateService { // 6. Return summary to display in a callout by the caller. return { trashed: toSoftDelete.length, permanentlyDeleted: toPermanentlyDelete.length }; } + + // ------------------------ + // Warnings accumulator helpers + // ------------------------ + private createWarningAccumulator(): WarningAccumulator { + return { + exactFallbackCount: 0, + unparseableUriCount: 0, + permissionDeniedCount: 0, + unparseableUris: [], + exactFallbackSamples: [], + permissionDeniedNames: [], + }; + } + + private toWarningsResult(acc: WarningAccumulator): DuplicateOperationWarnings { + return { + exactFallbackCount: acc.exactFallbackCount, + unparseableUriCount: acc.unparseableUriCount, + permissionDeniedCount: acc.permissionDeniedCount, + unparseableUris: acc.unparseableUris.length ? acc.unparseableUris : undefined, + exactFallbackSamples: acc.exactFallbackSamples.length ? acc.exactFallbackSamples : undefined, + permissionDeniedNames: acc.permissionDeniedNames.length + ? acc.permissionDeniedNames + : undefined, + }; + } +} + +interface WarningAccumulator { + exactFallbackCount: number; + unparseableUriCount: number; + permissionDeniedCount: number; + unparseableUris: string[]; + exactFallbackSamples: string[]; + permissionDeniedNames: string[]; } diff --git a/apps/web/src/locales/en/messages.json b/apps/web/src/locales/en/messages.json index 61a31423b8e..e2f30419105 100644 --- a/apps/web/src/locales/en/messages.json +++ b/apps/web/src/locales/en/messages.json @@ -11088,5 +11088,79 @@ "noValue": { "message": "-", "description": "Indicates that no value is present for a given item. First implemented when displaying a list of ciphers that may not belong to folders or organizations." + }, + "duplicateWarningsTitle": { + "message": "Some items were skipped or handled differently", + "description": "Title for a warning callout summarizing non-fatal issues encountered during de-duplication." + }, + "duplicateExactFallback": { + "message": "Exact matching failed for $COUNT$ URI.", + "placeholders": { + "count": { "content": "$1", "example": "3" } + }, + "description": "Warning indicating that due to parsing limitations, exact URI matching could not be guaranteed for some items." + }, + "duplicateExactFallbackPlural": { + "message": "Exact matching for for $COUNT$ URIs.", + "placeholders": { + "count": { "content": "$1", "example": "3" } + }, + "description": "Plural form of duplicateExactFallback." + }, + "duplicateUnparseableUris": { + "message": "$COUNT$ URI could not be parsed and was ignored.", + "placeholders": { + "count": { "content": "$1", "example": "5" } + }, + "description": "Warning indicating that some URIs could not be parsed and were excluded from duplicate detection." + }, + "duplicateUnparseableUrisPlural": { + "message": "$COUNT$ URIs could not be parsed and were ignored.", + "placeholders": { + "count": { "content": "$1", "example": "5" } + }, + "description": "Plural form of duplicateUnparseableUris." + }, + "duplicatePermissionDenied": { + "message": "$COUNT$ selected item could not be deleted due to permissions.", + "placeholders": { + "count": { "content": "$1", "example": "2" } + }, + "description": "Warning indicating that the user lacked permission to delete some of the selected duplicate items." + }, + "duplicatePermissionDeniedPlural": { + "message": "$COUNT$ selected items could not be deleted due to permissions.", + "placeholders": { + "count": { "content": "$1", "example": "2" } + }, + "description": "Plural form of duplicatePermissionDenied." + }, + "duplicateWarningsDialogTitle": { + "message": "De-duplication warnings", + "description": "Title of the dialog showing details for de-duplication warnings." + }, + "duplicateWarningsDialogExactHelp": { + "message": "Fix invalid URIs or try a less strict match strategy (Host/Hostname/Base).", + "description": "Helper sentence shown in the warnings details dialog to guide users about URI-related issues." + }, + "duplicateWarningsDialogPermHelp": { + "message": "Some items may be organization-owned or read-only. Contact an admin or adjust collection permissions.", + "description": "Helper sentence shown in the warnings details dialog to explain permission-denied deletions." + }, + "duplicateWarningsDetailsButton": { + "message": "View details", + "description": "Label for a button that opens a dialog with more information about de-duplication warnings." + }, + "duplicateWarningsDialogUnparseableTitle": { + "message": "Unparseable URIs", + "description": "Section title above the list of unparseable URIs." + }, + "duplicateWarningsDialogExactSamplesTitle": { + "message": "Exact matching fallbacks (sample)", + "description": "Section title above the sample of URIs where exact matching could not be guaranteed." + }, + "duplicateWarningsDialogPermissionDeniedTitle": { + "message": "Items not deleted (permission denied)", + "description": "Section title above the list of cipher names that could not be deleted due to permissions." } }