[BEEEP] Improve speed performance of Admin Console Collections page#20243
[BEEEP] Improve speed performance of Admin Console Collections page#20243
Conversation
… operations outside Angular's zone. Implement chunked decryption for ciphers and collections to prevent UI blocking. Introduce a long-lived Web Worker for managing the Lunr search index, improving search responsiveness and reducing main thread load. Update virtual scrolling cache size in vault items component for better rendering efficiency.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Re-reviewed after the three previously-flagged threads were resolved. The earlier concerns — missing Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #20243 +/- ##
==========================================
- Coverage 47.01% 46.97% -0.04%
==========================================
Files 3912 3913 +1
Lines 118440 118554 +114
Branches 18117 18130 +13
==========================================
+ Hits 55681 55688 +7
- Misses 58571 58674 +103
- Partials 4188 4192 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (5)Checkmarx found the following issues in this Pull Request
|
… Angular's zone for improved performance. Update VaultItemsComponent to use a getter/setter for allCollections, ensuring proper refresh of items. Enhance search.worker.ts with explicit extractors for login fields to maintain compatibility with nested object structures.
…p/improve-collections-page-speeds
…d of null for login fields, enhancing compatibility with nested object structures.
…or conditional statements in search results processing.
| } catch (e: unknown) { | ||
| self.postMessage({ type: "error", error: String(e) }); | ||
| } |
There was a problem hiding this comment.
Details and fix
When lunr throws inside the worker's search handler (e.g., user types an invalid query like >(), the worker posts { type: "error", error } without the requestId. On the main thread, getOrCreateWorker's onmessage only resolves pendingSearches entries when msg.type === "searchResults" — the "error" branch is ignored. The entry in pendingSearches is never deleted, so searchInWorker's Promise never settles, searchCiphers awaits forever, and isCipherSearching$ remains true (the searching spinner never clears).
The main-thread fallback path handles this exact case in search.service.ts (wraps index.search(...) in try/catch and logs), so the worker path is now less resilient than the code it replaced for malformed Lunr queries.
Suggested fix: include requestId on the error response and treat it as "empty results" on the main thread. For example in search.worker.ts:
} catch (e: unknown) {
self.postMessage({ type: "searchResults", requestId, results: [] });
// Optionally also post a separate diagnostic error message
}…and/or add an error branch in getOrCreateWorker's onmessage that resolves the matching pendingSearches entry with [].
| /** | ||
| * Long-lived Web Worker that owns the Lunr full-text search index for the entire session. | ||
| * | ||
| * WHY LONG-LIVED: The previous design built the index in a worker then serialised it and |
There was a problem hiding this comment.
@JaredScar KM has a concurrent piece of work that eliminates search indexing entirely in most cases, and probably already achieves your goal. It also restructures the search service massively, and conflict with this work. We should sync up on how to proceed
Unless you actually use lunr search queries, we should never build the search index, which is what the above work implements. Most users do not use lunr.
Two more notes here:
- @eliykat If we do move to the SDK, we can achieve much better performance than LUNR, even without multi-threading. LUNR is just incredibly inefficient at indexing. I would always suggest optimizing the actual work first before bringing in threading
- If we do want to merge both works, I would suggest merging [PM-32784] Increase search service search performance by 50x and defer indexing #19251 first, then adapting this work to work on top of the extracted lunr service. However, I don't actually know what this is intended to achieve because [PM-32784] Increase search service search performance by 50x and defer indexing #19251 already eliminates search indexing entirely except for the first LUNR query.
- If we do care about first LUNR query performance, then I suggest that we look into re-writing LUNR queries and indexes in the SDK
- Actually, this also applies to general LUNR query performance
However, a service worker always introduces a lot of complexity, and is probably just not needed with the approach taken in #19251. Could you test if #19251 already solves your issue?
There was a problem hiding this comment.
I'll also note that #19251 has been through multiple review rounds at this point, and partially through QA.
There was a problem hiding this comment.
Already discussed in Slack, but just for completeness - I agree we want to progress #19251 and rethink reintroducing multithreading per KM Team's advice.
| // Promise.all(8K) floods the microtask queue — decrypt in chunks with event-loop | ||
| // yields so the browser can paint and handle input between each batch. | ||
| const DECRYPT_CHUNK_SIZE = 2_000; | ||
| const results: CollectionAdminView[] = []; |
There was a problem hiding this comment.
Note: Porting state to the SDK will give better speed improvements.
…cipher fetch events. Introduce performance measurement for load duration and log debug messages upon load start and completion, improving observability of data retrieval processes.
|
…p/improve-collections-page-speeds
This change eliminates the unnecessary call to indexCiphers within the search logic, streamlining the process and improving performance. Additionally, the concatMap operator has been imported for potential future use.
3 participants
🎟️ Tracking
N/A
📔 Objective
Optimize performance of vault component and services by running heavy operations outside Angular's zone. Implement chunked decryption for ciphers and collections to prevent UI blocking. Introduce a long-lived Web Worker for managing the Lunr search index, improving search responsiveness and reducing main thread load. Update virtual scrolling cache size in vault items component for better rendering efficiency.
📸 Screenshots
https://vimeo.com/1184239975?share=copy&fl=sv&fe=ci