[PM-17487] Load cipher data into new notification experience #13185
Conversation
- Implement new getNotificationCipherData function in background - Update stories to match new naming - Edit types to be relevant to current functionality
|
New Issues (2)Checkmarx found the following issues in this Pull Request
Fixed Issues (1)Great job! The following issues were fixed in this Pull Request
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13185 +/- ##
==========================================
- Coverage 35.41% 35.05% -0.37%
==========================================
Files 3009 3036 +27
Lines 90815 92731 +1916
Branches 16877 16891 +14
==========================================
+ Hits 32163 32503 +340
- Misses 56185 57764 +1579
+ Partials 2467 2464 -3 ☔ View full report in Codecov by Sentry. |
jprusik
changed the title
| const currentTab = await BrowserApi.getTabFromCurrentWindow(); | ||
| const showFavicons = await firstValueFrom(this.domainSettingsService.showFavicons$); | ||
| const env = await firstValueFrom(this.environmentService.environment$); |
There was a problem hiding this comment.
small perf nit; if you want to allow async data fetching here, but still await the data collectively:
| const currentTab = await BrowserApi.getTabFromCurrentWindow(); | |
| const showFavicons = await firstValueFrom(this.domainSettingsService.showFavicons$); | |
| const env = await firstValueFrom(this.environmentService.environment$); | |
| const [currentTab, showFavicons, env] = await Promise.all([ | |
| BrowserApi.getTabFromCurrentWindow(), | |
| firstValueFrom(this.domainSettingsService.showFavicons$), | |
| firstValueFrom(this.environmentService.environment$), | |
| ]); |
|
TY - nice work! |
dan-livefront
deleted the
PM-17487-load-cipher-data-into-new-notification-experience
branch
| }), | ||
| document.body, | ||
| ); | ||
| sendPlatformMessage({ command: "bgGetDecryptedCiphers" }, (cipherData) => { |
There was a problem hiding this comment.
Note to future code archaeologists:
We discussed including this data with the initial notification data fetch (see "initNotificationBar" message), but opted to follow this pattern for now.
| * @returns {Promise<NotificationCipherData[]>} | ||
| */ | ||
|
|
||
| async getNotificationCipherData(): Promise<NotificationCipherData[]> { |
There was a problem hiding this comment.
Note: passing only cipher data that is actually needed/used in the Notification context (hence the narrow naming scope)
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-17487
📔 Objective
The objective of this PR is to get the relevant Cipher Data into the new notification bar experience, the approach taken was to create a
NotificationCipherDatatype that use the relevant cipher data for login for each url without the need of excess data. Leveraged existing functions and patterns.📸 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