[PM-3530] browser extension view cache #10437

willmartian · 2024-08-07T21:54:06Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-3530

📔 Objective

Split from #9556, see extra comments there.

Introduces a way to store temporary component state, for the purposes of persisting views between extension popup open and close.

ViewCacheService
- .signal, a wrapper around Angular Signals for caching arbitrary values

const mySignal = this.viewCacheService.signal({
   key: "popup-search-text"
   initialValue: "foo"
});

.formGroup, a wrapper around Angular Reactive Forms for caching dirty form values

this.loginDetailsForm = this.viewCacheService.formGroup({
      key: "vault-login-details-form",
      control: this.formBuilder.group({
        username: [""],
        password: [""],
        totp: [""],
      })
});

In both instances, the wrapped object's value is set with the previously cached value. Updates to the wrapped value are saved to the cache.

State is stored in the PopupViewCacheBackgroundService, which clears cached state on unlock, lock, tab change, & 2min after closing the popup window. State is also cleared when navigating between routes in the extension popup

Other 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 pagehide event occurs:

class Foo {
  name: string = getFromCache("foo-name", () => {
    return this.name; 
  });
}

However, pagehide (and visibilitychange/unload) do not consistently fire in Chrome extension windows. :(

Random thoughts

Why not a TS decorator? We might want to start watching the signal/form after performing other logic.
Do we need the deserializer field? See [PM-3530][PM-8588] persist extension route history #9556 (comment) but I could go either way.
We could unify the two methods into a single one: ViewCacheService.watch(formOrSignal, options)
- I just didn't think of this during my first pass. Dunno if it is better or less clear. I defer to reviewer.

📸 Screenshots

Screen.Recording.2024-07-02.at.9.58.27.AM.mov

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

…ilters

…ing; add route guard with warning dialog

codecov · 2024-08-13T02:30:51Z

Codecov Report

Attention: Patch coverage is 61.42857% with 27 lines in your changes missing coverage. Please review.

Project coverage is 32.91%. Comparing base (19ef6ba) to head (ff6e42a).
Report is 64 commits behind head on main.

✅ All tests successful. No failed tests found.

Files	Patch %	Lines
...rm/services/popup-view-cache-background.service.ts	38.46%	8 Missing ⚠️
...tform/popup/view-cache/popup-view-cache.service.ts	86.36%	4 Missing and 2 partials ⚠️
apps/browser/src/popup/app.component.ts	0.00%	4 Missing ⚠️
...r/src/platform/services/noop-view-cache.service.ts	0.00%	4 Missing ⚠️
apps/browser/src/popup/services/services.module.ts	0.00%	2 Missing ⚠️
libs/angular/src/services/jslib-services.module.ts	0.00%	2 Missing ⚠️
...ar/src/platform/abstractions/view-cache.service.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10437      +/-   ##
==========================================
+ Coverage   32.79%   32.91%   +0.11%     
==========================================
  Files        2670     2669       -1     
  Lines       81769    81824      +55     
  Branches    15414    15441      +27     
==========================================
+ Hits        26816    26929     +113     
+ Misses      52877    52805      -72     
- Partials     2076     2090      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coroiu

I wasn't around to review #9556 but from what I can see this looks good. It's gonna take some time to get used to signals, especially when used in a complex scenario like this

coroiu · 2024-08-21T08:49:07Z

libs/angular/src/platform/services/view-cache.service.ts

+  /**
+   * - Initialize a form from a cached value
+   * - Save form value to cache when it changes
+   * - The form is marked dirty if the restored value is not `undefined`.
+   **/
+  formGroup<TFormGroup extends FormGroup>(options: FormCacheOptions<TFormGroup>): TFormGroup {
+    return options.control;
+  }


question: This isn't actually true right? This is the behavior that's implemented by the popup version of this service, but not by this general service?

Good question @coroiu

Yes, the default implementation is noop. However, since we inject via the default implementation, I thought it would be more helpful to document the non-noop behavior so it will show up in IDE-hints.

Is this a bad idea, or is there a more clear way that you would suggest representing this?

Maybe just add Non browser extension implementations are noop. ?

I think adding your comment about non browser extensions is definitely a good idea.

Added here 8ac50e4

I think I reacted because this was documenting an implementation which was obviously not providing that behavior. I'm not sure if we normally use classes as interfaces AND default implementations like this. I feel like I mostly see the interface defined and documented separately. That way this noop implementation could clearly document the fact that it is not living up to the interface documentation, and why that doesn't matter.

I think that is a good point! Added 86b05f8 and 556683a.

Also provided the default implementation to other clients here: 1f22860

justindbaur

I think the only thing I need is a comment on the formGroup method mentioning its nuance.

willmartian · 2024-08-22T01:51:04Z

Was discussing the feature flag strategy for this with @trmartin4 and we thought it might be nice to include the feature flag in the service logic instead of expecting consumers to flag it manually. Lmk what y'all think @justindbaur @coroiu.

811761b

coroiu · 2024-08-22T07:54:28Z

apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts

+  private updateState(key: string, value: string) {
+    this.messageSender.send(SAVE_VIEW_CACHE_COMMAND, {
+      key,
+      value,
+    });
+  }


question: maybe we should disable this with the feature flag as well?

Seems reasonable to me. Added ff6e42a

willmartian added 30 commits June 9, 2024 14:55

[PM-8589] persist dirty form data (wip)

dbbf307

[PM-8589] serialize value comparsion in CL select components

c887bab

[PM-8589] test implementation in send-add-edit and vault-popup-list-f…

96a71fc

…ilters

[PM-8588] persist extension route history (wip)

8f9bb2d

temp: override feature flag

64e6bab

gate behind FeatureFlag.ExtensionRefresh

4185010

share single StateDefinition

286f909

use large object storage; use global state provider with manual clear…

0c25260

…ing; add route guard with warning dialog

add new feature flag

38a6c32

wip

9b4ac8f

wip; add tests for guard

e432d28

resolve merge conflict

6d4d97b

add flag to service back method

1a6eb9f

wip

13d3e40

add tests; fix bug

d88e7e8

rename

f274a02

resolve merge conflicts

ff93cd0

remove feature flag override

b033a70

revert usage examples

d6ef9c0

fix merge error

6a0d99a

resolve merge conflicts

6a97de6

remove unecessary call to clear state

76b4c94

fix account switch clear state after latest merge commit

8d2aab9

save route history to URL param when building popout url

e2a3863

resolve merge conflicts

aded1e5

cr: remove promise in subscribe; remove comment from test

28431e2

vicki cr changes

c07b840

add back method test

33f86d0

fix popup utils tests

2541915

remove 'T' prefix from type names

a732ef5

willmartian changed the title ~~Revert "remove view cache references to split PR"~~ [PM-3530] browser extension view cache Aug 13, 2024

revert browser-popup-utils and pop-out component

431d942

add to services module

afd12db

willmartian added the hold do not merge, do not approve yet label Aug 20, 2024

willmartian added 2 commits August 19, 2024 23:57

remove DS change

1ff8b48

remove temp changes

a557ec4

willmartian removed the hold do not merge, do not approve yet label Aug 20, 2024

Merge branch 'main' into ps/PM-3530/view-cache

a8e2af7

willmartian marked this pull request as ready for review August 20, 2024 21:15

willmartian requested a review from a team as a code owner August 20, 2024 21:15

willmartian requested review from coroiu and justindbaur August 20, 2024 21:15

coroiu previously approved these changes Aug 21, 2024

View reviewed changes

justindbaur reviewed Aug 21, 2024

View reviewed changes

update comment

8ac50e4

willmartian dismissed coroiu’s stale review via 8ac50e4 August 21, 2024 18:18

willmartian requested review from justindbaur and coroiu August 21, 2024 18:19

add feature flag

811761b

coroiu reviewed Aug 22, 2024

View reviewed changes

willmartian added 4 commits August 22, 2024 10:16

split noop implementation and abstract class

86b05f8

add comments

556683a

add to jslib services module

1f22860

add flag to update state

ff6e42a

willmartian requested a review from coroiu August 22, 2024 14:33

justindbaur approved these changes Aug 26, 2024

View reviewed changes

willmartian merged commit 6918606 into main Aug 26, 2024
66 checks passed

willmartian deleted the ps/PM-3530/view-cache branch August 26, 2024 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-3530] browser extension view cache #10437

[PM-3530] browser extension view cache #10437

willmartian commented Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading

coroiu left a comment

coroiu Aug 21, 2024

willmartian Aug 21, 2024

justindbaur Aug 21, 2024

willmartian Aug 21, 2024

coroiu Aug 22, 2024

willmartian Aug 22, 2024

willmartian Aug 22, 2024

justindbaur left a comment

willmartian commented Aug 22, 2024

coroiu Aug 22, 2024

willmartian Aug 22, 2024

[PM-3530] browser extension view cache #10437

[PM-3530] browser extension view cache #10437

Conversation

willmartian commented Aug 7, 2024 • edited Loading

🎟️ Tracking

📔 Objective

Other approaches

Random thoughts

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Aug 13, 2024 • edited Loading

Codecov Report

coroiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justindbaur left a comment

Choose a reason for hiding this comment

willmartian commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willmartian commented Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 13, 2024 •

edited

Loading