diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.ts index fe2baf463cf..72df3cba41a 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.ts @@ -5,12 +5,11 @@ import { FormsModule } from "@angular/forms"; import { Subject, Subscription, debounceTime, distinctUntilChanged, filter } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; +import { SearchTextDebounceInterval } from "@bitwarden/common/vault/services/search.service"; import { SearchModule } from "@bitwarden/components"; import { VaultPopupItemsService } from "../../../services/vault-popup-items.service"; -const SearchTextDebounceInterval = 200; - @Component({ imports: [CommonModule, SearchModule, JslibModule, FormsModule], selector: "app-vault-v2-search", diff --git a/apps/desktop/src/vault/app/vault/vault-items-v2.component.ts b/apps/desktop/src/vault/app/vault/vault-items-v2.component.ts index 000281c5807..e99e05e385a 100644 --- a/apps/desktop/src/vault/app/vault/vault-items-v2.component.ts +++ b/apps/desktop/src/vault/app/vault/vault-items-v2.component.ts @@ -2,7 +2,7 @@ import { ScrollingModule } from "@angular/cdk/scrolling"; import { CommonModule } from "@angular/common"; import { Component } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { distinctUntilChanged } from "rxjs"; +import { distinctUntilChanged, debounceTime } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { VaultItemsComponent as BaseVaultItemsComponent } from "@bitwarden/angular/vault/components/vault-items.component"; @@ -10,6 +10,7 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { SearchService } from "@bitwarden/common/vault/abstractions/search.service"; import { RestrictedItemTypesService } from "@bitwarden/common/vault/services/restricted-item-types.service"; +import { SearchTextDebounceInterval } from "@bitwarden/common/vault/services/search.service"; import { CipherViewLike, CipherViewLikeUtils, @@ -35,7 +36,7 @@ export class VaultItemsV2Component extends BaseVaultIt super(searchService, cipherService, accountService, restrictedItemTypesService); this.searchBarService.searchText$ - .pipe(distinctUntilChanged(), takeUntilDestroyed()) + .pipe(debounceTime(SearchTextDebounceInterval), distinctUntilChanged(), takeUntilDestroyed()) .subscribe((searchText) => { this.searchText = searchText!; }); diff --git a/apps/web/src/app/vault/individual-vault/vault.component.ts b/apps/web/src/app/vault/individual-vault/vault.component.ts index 7593e9c59bf..35520a812a8 100644 --- a/apps/web/src/app/vault/individual-vault/vault.component.ts +++ b/apps/web/src/app/vault/individual-vault/vault.component.ts @@ -68,6 +68,7 @@ import { TreeNode } from "@bitwarden/common/vault/models/domain/tree-node"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { ServiceUtils } from "@bitwarden/common/vault/service-utils"; import { RestrictedItemTypesService } from "@bitwarden/common/vault/services/restricted-item-types.service"; +import { SearchTextDebounceInterval } from "@bitwarden/common/vault/services/search.service"; import { CipherViewLike, CipherViewLikeUtils, @@ -137,7 +138,6 @@ import { VaultHeaderComponent } from "./vault-header/vault-header.component"; import { VaultOnboardingComponent } from "./vault-onboarding/vault-onboarding.component"; const BroadcasterSubscriptionId = "VaultComponent"; -const SearchTextDebounceInterval = 200; @Component({ selector: "app-vault", diff --git a/libs/angular/src/vault/components/vault-items.component.ts b/libs/angular/src/vault/components/vault-items.component.ts index 75ca5608208..8ae4acfb687 100644 --- a/libs/angular/src/vault/components/vault-items.component.ts +++ b/libs/angular/src/vault/components/vault-items.component.ts @@ -1,18 +1,16 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore -import { Directive, EventEmitter, Input, OnDestroy, OnInit, Output } from "@angular/core"; +import { Directive, EventEmitter, Input, OnDestroy, Output } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { BehaviorSubject, Subject, combineLatest, filter, - from, map, of, shareReplay, switchMap, - takeUntil, } from "rxjs"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; @@ -29,7 +27,7 @@ import { } from "@bitwarden/common/vault/utils/cipher-view-like-utils"; @Directive() -export class VaultItemsComponent implements OnInit, OnDestroy { +export class VaultItemsComponent implements OnDestroy { @Input() activeCipherId: string = null; @Output() onCipherClicked = new EventEmitter(); @Output() onCipherRightClicked = new EventEmitter(); @@ -55,12 +53,9 @@ export class VaultItemsComponent implements OnInit, On shareReplay({ bufferSize: 1, refCount: true }), ); - protected searchPending = false; - /** Construct filters as an observable so it can be appended to the cipher stream. */ private _filter$ = new BehaviorSubject<(cipher: C) => boolean | null>(null); private destroy$ = new Subject(); - private isSearchable: boolean = false; private _searchText$ = new BehaviorSubject(""); get searchText() { @@ -87,19 +82,6 @@ export class VaultItemsComponent implements OnInit, On this.subscribeToCiphers(); } - async ngOnInit() { - combineLatest([getUserId(this.accountService.activeAccount$), this._searchText$]) - .pipe( - switchMap(([userId, searchText]) => - from(this.searchService.isSearchable(userId, searchText)), - ), - takeUntil(this.destroy$), - ) - .subscribe((isSearchable) => { - this.isSearchable = isSearchable; - }); - } - ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); @@ -140,10 +122,6 @@ export class VaultItemsComponent implements OnInit, On this.onAddCipherOptions.emit(); } - isSearching() { - return !this.searchPending && this.isSearchable; - } - protected deletedFilter: (cipher: C) => boolean = (c) => CipherViewLikeUtils.isDeleted(c) === this.deleted; diff --git a/libs/common/src/vault/abstractions/search.service.ts b/libs/common/src/vault/abstractions/search.service.ts index 57f301261c2..6b01302613c 100644 --- a/libs/common/src/vault/abstractions/search.service.ts +++ b/libs/common/src/vault/abstractions/search.service.ts @@ -9,7 +9,12 @@ export abstract class SearchService { abstract indexedEntityId$(userId: UserId): Observable; abstract clearIndex(userId: UserId): Promise; - abstract isSearchable(userId: UserId, query: string): Promise; + + /** + * Checks if the query is long enough to be searchable. + * For short Lunr.js queries (starts with '>'), a valid search index must exist for the user. + */ + abstract isSearchable(userId: UserId, query: string | null): Promise; abstract indexCiphers( userId: UserId, ciphersToIndex: CipherView[], diff --git a/libs/common/src/vault/services/cipher.service.ts b/libs/common/src/vault/services/cipher.service.ts index 2f225d4dfc5..60e5bfe04e4 100644 --- a/libs/common/src/vault/services/cipher.service.ts +++ b/libs/common/src/vault/services/cipher.service.ts @@ -183,8 +183,7 @@ export class CipherService implements CipherServiceAbstraction { ]).pipe( filter(([ciphers, _, keys]) => ciphers != null && keys != null), // Skip if ciphers haven't been loaded yor synced yet switchMap(() => this.getAllDecrypted(userId)), - tap(async (decrypted) => { - await this.searchService.indexCiphers(userId, decrypted); + tap(() => { this.messageSender.send("updateOverlayCiphers"); }), ); @@ -216,7 +215,7 @@ export class CipherService implements CipherServiceAbstraction { if (value == null) { await this.searchService.clearIndex(userId); } else { - await this.searchService.indexCiphers(userId, value); + void this.searchService.indexCiphers(userId, value); } } } diff --git a/libs/common/src/vault/services/search.service.spec.ts b/libs/common/src/vault/services/search.service.spec.ts new file mode 100644 index 00000000000..9df6ec5ce8d --- /dev/null +++ b/libs/common/src/vault/services/search.service.spec.ts @@ -0,0 +1,97 @@ +import { BehaviorSubject } from "rxjs"; + +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { FakeStateProvider, mockAccountServiceWith } from "../../../spec"; + +import { SearchService } from "./search.service"; + +describe("SearchService", () => { + let fakeStateProvider: FakeStateProvider; + let service: SearchService; + + const userId = "user-id" as UserId; + const mockLogService = { + error: jest.fn(), + measure: jest.fn(), + }; + const mockLocale$ = new BehaviorSubject("en"); + const mockI18nService = { + locale$: mockLocale$.asObservable(), + }; + + beforeEach(() => { + jest.clearAllMocks(); + + fakeStateProvider = new FakeStateProvider(mockAccountServiceWith(userId)); + service = new SearchService( + mockLogService as unknown as LogService, + mockI18nService as unknown as I18nService, + fakeStateProvider, + ); + }); + + describe("isSearchable", () => { + let mockIndex$: jest.Mock; + beforeEach(() => { + mockIndex$ = jest.fn(); + service["index$"] = mockIndex$; + }); + + it("returns false if the query is empty", async () => { + const result = await service.isSearchable(userId, ""); + expect(result).toBe(false); + // Ensure we do not call the expensive index$ method + expect(mockIndex$).not.toHaveBeenCalled(); + }); + + it("returns false if the query is null", async () => { + const result = await service.isSearchable(userId, null as any); + expect(result).toBe(false); + // Ensure we do not call the expensive index$ method + expect(mockIndex$).not.toHaveBeenCalled(); + }); + + it("return true if the query is longer than searchableMinLength", async () => { + service["searchableMinLength"] = 3; + const result = await service.isSearchable(userId, "test"); + expect(result).toBe(true); + // Ensure we do not call the expensive index$ method + expect(mockIndex$).not.toHaveBeenCalled(); + }); + + it("returns false if the query is shorter than searchableMinLength", async () => { + service["searchableMinLength"] = 5; + const result = await service.isSearchable(userId, "test"); + expect(result).toBe(false); + // Ensure we do not call the expensive index$ method + expect(mockIndex$).not.toHaveBeenCalled(); + }); + + it("returns false for short Lunr query with missing index", async () => { + mockIndex$.mockReturnValue(new BehaviorSubject(null)); + service["searchableMinLength"] = 3; + const result = await service.isSearchable(userId, ">l"); + expect(result).toBe(false); + expect(mockIndex$).toHaveBeenCalledWith(userId); + }); + + it("returns false for long Lunr query with missing index", async () => { + mockIndex$.mockReturnValue(new BehaviorSubject(null)); + service["searchableMinLength"] = 3; + const result = await service.isSearchable(userId, ">longer"); + expect(result).toBe(false); + expect(mockIndex$).toHaveBeenCalledWith(userId); + }); + + it("returns true for short Lunr query with index", async () => { + mockIndex$.mockReturnValue(new BehaviorSubject(true)); + service["searchableMinLength"] = 3; + const result = await service.isSearchable(userId, ">l"); + expect(result).toBe(true); + expect(mockIndex$).toHaveBeenCalledWith(userId); + }); + }); +}); diff --git a/libs/common/src/vault/services/search.service.ts b/libs/common/src/vault/services/search.service.ts index 614fba4a7ca..cb6ac4a0ed4 100644 --- a/libs/common/src/vault/services/search.service.ts +++ b/libs/common/src/vault/services/search.service.ts @@ -4,6 +4,8 @@ import * as lunr from "lunr"; import { Observable, firstValueFrom, map } from "rxjs"; import { Jsonify } from "type-fest"; +import { perUserCache$ } from "@bitwarden/common/vault/utils/observable-utilities"; + import { UriMatchStrategy } from "../../models/domain/domain-service"; import { I18nService } from "../../platform/abstractions/i18n.service"; import { LogService } from "../../platform/abstractions/log.service"; @@ -21,6 +23,9 @@ import { CipherType } from "../enums/cipher-type"; import { CipherView } from "../models/view/cipher.view"; import { CipherViewLike, CipherViewLikeUtils } from "../utils/cipher-view-like-utils"; +// Time to wait before performing a search after the user stops typing. +export const SearchTextDebounceInterval = 200; // milliseconds + export type SerializedLunrIndex = { version: string; fields: string[]; @@ -101,11 +106,19 @@ export class SearchService implements SearchServiceAbstraction { return this.stateProvider.getUser(userId, LUNR_SEARCH_INDEX); } - private index$(userId: UserId): Observable { + private index$ = perUserCache$((userId: UserId) => { return this.searchIndexState(userId).state$.pipe( - map((searchIndex) => (searchIndex ? lunr.Index.load(searchIndex) : null)), + map((searchIndex) => { + let index: lunr.Index | null = null; + if (searchIndex) { + const loadTime = performance.now(); + index = lunr.Index.load(searchIndex); + this.logService.measure(loadTime, "Vault", "SearchService", "index load"); + } + return index; + }), ); - } + }); private searchIndexEntityIdState(userId: UserId): SingleUserState { return this.stateProvider.getUser(userId, LUNR_SEARCH_INDEXED_ENTITY_ID); @@ -129,17 +142,22 @@ export class SearchService implements SearchServiceAbstraction { await this.searchIsIndexingState(userId).update(() => null); } - async isSearchable(userId: UserId, query: string): Promise { - const time = performance.now(); + async isSearchable(userId: UserId, query: string | null): Promise { query = SearchService.normalizeSearchQuery(query); - const index = await this.getIndexForSearch(userId); - const notSearchable = - query == null || - (index == null && query.length < this.searchableMinLength) || - (index != null && query.length < this.searchableMinLength && query.indexOf(">") !== 0); - this.logService.measure(time, "Vault", "SearchService", "isSearchable"); - return !notSearchable; + // Nothing to search if the query is null + if (query == null || query === "") { + return false; + } + + const isLunrQuery = query.indexOf(">") === 0; + if (isLunrQuery) { + // Lunr queries always require an index + return (await this.getIndexForSearch(userId)) != null; + } + + // Regular queries only require a minimum length + return query.length >= this.searchableMinLength; } async indexCiphers( @@ -205,6 +223,7 @@ export class SearchService implements SearchServiceAbstraction { ciphers: C[], ): Promise { const results: C[] = []; + const searchStartTime = performance.now(); if (query != null) { query = SearchService.normalizeSearchQuery(query.trim().toLowerCase()); } @@ -236,7 +255,9 @@ export class SearchService implements SearchServiceAbstraction { const index = await this.getIndexForSearch(userId); if (index == null) { // Fall back to basic search if index is not available - return this.searchCiphersBasic(ciphers, query); + const basicResults = this.searchCiphersBasic(ciphers, query); + this.logService.measure(searchStartTime, "Vault", "SearchService", "basic search complete"); + return basicResults; } const ciphersMap = new Map(); @@ -270,6 +291,7 @@ export class SearchService implements SearchServiceAbstraction { } }); } + this.logService.measure(searchStartTime, "Vault", "SearchService", "search complete"); return results; }