[PM-17094] - Vault and Collection filters pop in after Vault content causing content to shift #13165
Conversation
jaasen-livefront
changed the title
|
New Issues (4)Checkmarx found the following issues in this Pull Request
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13165 +/- ##
===========================================
+ Coverage 15.76% 35.31% +19.54%
===========================================
Files 27 3128 +3101
Lines 1884 92603 +90719
Branches 0 16826 +16826
===========================================
+ Hits 297 32702 +32405
- Misses 1587 57441 +55854
- Partials 0 2460 +2460 ☔ View full report in Codecov by Sentry. |
| @@ -92,13 +93,16 @@ export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { | |||
|
|
|||
| private allFilters$ = this.vaultPopupListFiltersService.allFilters$; | |||
|
|
|||
| protected listFiltersLoaded$ = this.vaultListFilterLoadingStateService.filtersLoaded$; | |||
There was a problem hiding this comment.
❓ Do we need a separate service? I would suspect we can just hook into the existing this.allFilters$ above and use it in the *ngIf in the template.
Maybe there is something else I'm missing?
There was a problem hiding this comment.
@shane-melton That was my first try but it appears the issue seems to lie in the vault list filters component and the fact that it renders after the main content. The only way to ensure the main content loads with it is to wait for allFilters$ to emit in the filter component. I removed the call to delay(0) and requestAnimationFrame as that isn't required. I'm happy to pair up or chat if you want to go over it in more detail.
There was a problem hiding this comment.
My mind is on the same path as @shane-melton, I believe the solution is correct but the service to manage the "loaded" state isn't needed, right?
listFiltersLoaded$ could use the allFilters$ directly:
protected listFiltersLoaded$ = this.allFilters$.pipe(
take(1),
takeUntilDestroyed(this.destroyRef),
);There was a problem hiding this comment.
I did a bit more digging into to this yesterday and I believe I found the cause of the problem you were having @jaasen-livefront.
The vault popup filter service does not use a shareReplay() for its organizations$, folders$, and collections$ observables. So, those values are recomputed for every subscriber, which includes the vault filter section component, which is rendered after the main content as you mention, and so it has a slight delay for the second computation of those observables.
When I added the shareReplay() to those 3 base observables in the filter service AND update the loading$ observable in the vault-v2 component to also check if allFilters$ has emitted the vault loads all at once instead of popping in piece by piece.
There was a problem hiding this comment.
@shane-melton Wow, so much simpler. Thank you!
There was a problem hiding this comment.
AND update the loading$ observable in the vault-v2 component to also check if allFilters$ has emitted the vault loads all at once instead of popping in piece by piece.
@jaasen-livefront I believe this piece still needs to be implemented from @shane-melton's feedback otherwise this is looking good!
There was a problem hiding this comment.
@nick-livefront Done!
| private vaultListFilterLoadingStateService: VaultListFilterLoadingStateService, | ||
| ) {} | ||
|
|
||
| private destroy$ = new Subject<void>(); |
There was a problem hiding this comment.
destory$.next() so the takeUntil operator below never triggers.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-17094
📔 Objective
Given the vault list filter component is relying on async data it's loading after the main content and causing it to shift. This PR adds a
shareReplayfunction to share the filters across all the observers to ensure they all render together.📸 Screenshots
Screen.Recording.2025-01-30.at.11.31.59.AM.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