[PM-3530] persist extension popup view #9556
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9556 +/- ##
==========================================
+ Coverage 29.56% 29.66% +0.10%
==========================================
Files 2538 2551 +13
Lines 74166 74626 +460
Branches 13855 13947 +92
==========================================
+ Hits 21925 22138 +213
- Misses 50582 50827 +245
- Partials 1659 1661 +2 ☔ View full report in Codecov by Sentry. |
|
No New Or Fixed Issues Found |
|
Hold until 64e6bab is removed. edit: done |
…ing; add route guard with warning dialog
|
I think I would prefer to accept only primitive types in the cache, such as |
There was a problem hiding this comment.
❌ Seems like this could leak arbitrary secrets from a view into plaintext global state.
🤔 This feature seems like it would be better handled on a case-by-case basis in the service rather than as a globally-available feature. The 🎨 item explains how I'd hoped to add this feature to sends.
🎨 Adding a save buffer is fairly easy to support with BufferedState. I think spending energy adding missing features to it will produce better outcomes, since as a state provider it's intended to be wrapped by services that can be fully-conscious of the security and consistency requirements of the data. For example, the buffer includes clearon flags so that you can control when save state is lost if necessary. It also includes a dependency that toggles when its downstream state is overwritten.
👍🏻 I'd like to note that solving the problem is awesome, I'm all for that. I just know from experience that a global UI state cache makes doing the wrong thing really easy, and doing the right thing very complex.
📝 A great example of that is how @vleague2 is talking about primitive storage and building a mapping layer. rxjs already solves arbitrary mapping with map, and because that's just a function the mapping is fast. It also blends very nicely with reactive controls.
// example output
svc.saveBuffer$(editing.id).pipe(
map(i => ({ ...i, foo: i.bar }),
).subscribe(myFormGroup.patch);💭 If you really want a "global" store, you could surface it as a service that surfaces rx interfaces.
// `TemporaryStateService`
// * Wraps state providers with RxJs subjects
// * Is backed by state providers that always clears on logout
const tss: TemporaryStateService = // construction or DI or whatever
tss.subject("myPageName"); // default protected by account key; fails if account locked
tss.subject("myPageName", { kind: "memory-only" }); // this kind never persists
tss.subject("myPageName", { kind: "plaintext" }); // this kind saves w/o encryption
tss.subject("myPageName", { clearon: ["leave"], }); // new clearon: user navigates away
tss.subject("myPageName", { kind: "device" }); // this kind is device key encrypted (if supported)
// rxjs subject supports subscription both ways
const tsub = tss.subject("myPageName");
tsub.value$.subscribe(myFormGroup.patchValue);
myFormGroup.valueChanges.subscribe(tsub.next);
// similar could be done for signals, though that would draw a dependency on Angular's implementation atm
tss.signal(/* same args as subject */)
| const updateUrl = !child?.data?.doNotSaveUrl ?? true; | ||
|
|
||
| if (updateUrl) { | ||
| void this.push(event.url); |
| )) as UrlTree; | ||
|
|
||
| expect(serializer.serialize(response)).toBe("/b"); | ||
| // expect(await service.last()).toBe("/a"); |
There was a problem hiding this comment.
Is this a leftover that can be deleted?
|
|
||
| expect(serializer.serialize(response)).toBe("/b"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Might be worth adding a test for the service's back function since that gets used directly outside of the cache guard.
| }); | ||
|
|
||
| const value = _signal(); | ||
| if (value != null && JSON.stringify(value) !== JSON.stringify(control.getRawValue())) { |
There was a problem hiding this comment.
Any reason to not use strict equality here? !==
| } | ||
|
|
||
| /** | ||
| * Push new route onto history stack |
There was a problem hiding this comment.
For these methods that return booleans, it might help to document what the return values signify.
| testBed.inject(PopupRouterCacheService); | ||
| }); | ||
|
|
||
| it("returns true if empty", async () => { |
There was a problem hiding this comment.
Just to add a little more context:
| it("returns true if empty", async () => { | |
| it("returns true if the history stack is empty", async () => { |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-3530
📔 Objective
Adds a few utilities for persisting the extension popup view:
PopupRouterCacheServicepopupRouterCacheGuardFeatureFlag.PersistPopupViewPopupViewCacheServicecacheSignal, for caching arbitrary valuescacheFormGroup, for caching dirty form valuesPopupViewCacheBackgroundServicemisc changes:
chip-select.component.ts&select.component.tsis not restricted to generics, and was previously being checked for equality with===. This no longer works because the value that is grabbed from state will not be the same object reference:FolderView.fromJson({ ... }) !== FolderView.fromJson({ ... })One way to fix this is to JSON.serialize both sides of the equality check, which I did here. However, devs may run into issues if they are checking for object equality elsewhere in their code.
Other options include:
cacheSignalandcacheFormGroupdeserializeroption entirelydeserializer:deserializerneeds to be asyncOther approaches
I initially wanted to utilize a simpler, non-reactive API like the following, which accepts a cleanup function that runs when the popup's
pagehideevent occurs:However,
pagehide(andvisibilitychange/unload) do not consistently fire in Chrome extension windows. :(📸 Screenshots
Recording based on #9911 (I recommend reviewers take a look)
Screen.Recording.2024-07-02.at.9.58.27.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