[PM-17658] Fix persist route to clear if service worker dies #13382
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13382 +/- ##
==========================================
+ Coverage 35.20% 35.31% +0.10%
==========================================
Files 3126 3128 +2
Lines 92567 92610 +43
Branches 16857 16827 -30
==========================================
+ Hits 32590 32705 +115
+ Misses 57520 57445 -75
- Partials 2457 2460 +3 ☔ View full report in Codecov by Sentry. |
|
New Issues (4)Checkmarx found the following issues in this Pull Request
Fixed Issues (3)Great job! The following issues were fixed in this Pull Request
|
| return new TaskSchedulerSheduler(taskScheduler, taskName); | ||
| } | ||
|
|
||
| class TaskSchedulerSheduler implements SchedulerLike { |
There was a problem hiding this comment.
typo?: SchedulerScheduler ?
There was a problem hiding this comment.
Haha no... but I am definitely open to other names. It's an implementation of Scheduler based on our service named TaskSchedulerService (but I kind of wish we had dropped the Service on it. So I prefixed the thing it is with its modifier. The type name is an implementation detail and not exposed anywhere if that changes any of your feelings.
There was a problem hiding this comment.
Oh right, it's a Scheduler that belong to the TaskScheduler, I don't have any better suggestion, except maybe just Scheduler 🤷
There was a problem hiding this comment.
I think I kind of like TaskSchedulerScheduler more at least as a thing that doesn't have to be consumed by other directly.
| * @param taskName The name of the task that the handler should be registered and scheduled based on. | ||
| * @returns A SchedulerLike object that can be passed in to RXJS operators like `delay` and `timeout`. | ||
| */ | ||
| export function toScheduler( |
There was a problem hiding this comment.
suggestion: could you add an @example here with some documentation about the expected behavior?
There was a problem hiding this comment.
Added an example a little bit more of an explanation on our side and a couple links to helpful guides on https://rxjs.dev, anything else you think would be nice?
| switchMap(([port]) => fromChromeEvent(port.onDisconnect).pipe(delay(1000 * 60 * 2))), | ||
| switchMap(([port]) => | ||
| fromChromeEvent(port.onDisconnect).pipe( | ||
| delay( | ||
| 1000 * 60 * 2, | ||
| toScheduler(this.taskSchedulerService, ScheduledTaskNames.clearPopupViewCache), | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
question: Do you know why we are delaying these emissions inside of a nested pipe? Are we implementing our own debounce? Could this be rewritten like this: ?
switchMap(([port]) => fromChromeEvent(port.onDisconnect)),
debounceTime(
1000 * 60 * 2,
toScheduler(this.taskSchedulerService, ScheduledTaskNames.clearPopupViewCache)
),or am I missing some subtlety?
There was a problem hiding this comment.
It looks like it shouldn't need to be nested but it's not to try and achieve a debounce. Although a debounce would probably work fine also. This is just trying to clear the view cache 2 minutes after a popup closes, an event that can't happen that much (without opening a new one which should cancel the delay), so it's not needed either.
There was a problem hiding this comment.
Ooh ok we're not debouncing, we just want to wait 2 minutes UNLESS a new window appears in which case we want to cancel the timeout. Looks good in that case, clever but not immediately obvious :)
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-17658
📔 Objective
Fix
delayto still trigger a cleanup even in the event that the service worker dies. By using aSchedulerLikebased on our ownTaskSchedulerServiceit will utilize a more long lived scheduling infrastructure when needed. When it's not needed it will still usesetTimeoutunder the hood, just like the default scheduler in rxjs.📸 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