[PM-16133] - [Vault] Implement persistence on filters in Vault view #13303
Conversation
jaasen-livefront
requested review from
nick-livefront,
shane-melton and
gbubemismith
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13303 +/- ##
==========================================
- Coverage 35.50% 35.13% -0.37%
==========================================
Files 3010 3031 +21
Lines 90934 92549 +1615
Branches 16915 16853 -62
==========================================
+ Hits 32282 32518 +236
- Misses 56149 57571 +1422
+ Partials 2503 2460 -43 ☔ View full report in Codecov by Sentry. |
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
| @@ -1,5 +1,3 @@ | |||
| // FIXME: Update this file to be type safe and remove this and next line | |||
| // @ts-strict-ignore | |||
There was a problem hiding this comment.
👏 Thanks for fixing this as you passed by!
| @@ -282,8 +286,8 @@ export class VaultPopupItemsService { | |||
| private accountService: AccountService, | |||
| ) {} | |||
|
|
|||
| applyFilter(newSearchText: string) { | |||
| this._searchText$.next(newSearchText); | |||
| applyFilter(newSearchText: string | null) { | |||
There was a problem hiding this comment.
🤔 applyFilter is a little ambiguous in the context of the chip filters. Maybe another name for the method would make it more intuitive? applySearchFilter, updateSearchText?
No change needed if you don't agree
| toObservable(cachedFilters) | ||
| .pipe(filter((state) => Object.keys(state).length > 0)) | ||
| .subscribe((state) => this.deserializeFilters(state)); |
There was a problem hiding this comment.
❓ Does this need to be an observable if it is only supposed to load the initial filters?
this.deserializeFilters(cachedFilters());| const FILTER_VISIBILITY_KEY = new KeyDefinition<boolean>(VAULT_SETTINGS_DISK, "filterVisibility", { | ||
| deserializer: (obj) => obj, | ||
| }); | ||
|
|
||
| export interface CachedFilterState { |
There was a problem hiding this comment.
⛏️ Could you add a comment about why the CachedFilterState differs from the internal state?
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16133
📔 Objective
This PR introduces persistence of the vault filters and search function. It leverages the
PopupViewCacheServiceas the caching mechanism.📸 Screenshots
Screen.Recording.2025-02-06.at.4.44.28.PM.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes