-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PM-12759] - Admin Console - Link to vault cipher is not opening cipher modal #12738
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #12738 +/- ##
=======================================
Coverage 34.14% 34.15%
=======================================
Files 2937 2937
Lines 90340 90345 +5
Branches 16963 16964 +1
=======================================
+ Hits 30846 30853 +7
+ Misses 57039 57037 -2
Partials 2455 2455 ☔ View full report in Codecov by Sentry. |
// Only process the queryParams if the dialog is not open (only when extension refresh is enabled) | ||
filter(() => this.vaultItemDialogRef == undefined || !this.extensionRefreshEnabled), | ||
withLatestFrom(allCipherMap$, allCollections$, organization$), | ||
switchMap(() => combineLatest([this.route.queryParams, allCipherMap$])), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe switching this to combineLatest
is going to have multiple emissions from the observable. The withLatestFrom
was to address this bug previously PM-12678
this change also seems to result in the edit modal being called multiple times. Screen recording below:
PM-12759.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jingo88 Ok I've added some logic to prevent double rendering. Let me know what you think!
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12759
📔 Objective
This PR fixes an issue where on initial load the org vault cipher dialog was not opening even when
itemId
andaction=view
params were present. This was due to the use ofwithLatestFrom
which was firing on first emission of the query param observable but the array of observables passed in (allCipherMap$, allCollections$, organization$
) had not yet emitted and thus the stream was halted. UsingswitchMap
ensures all observables have emitted before running.Steps to test for success:
📸 Screenshots
⏰ 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