[PM-3530] persist extension popup view

[willmartian]

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ 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

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 61.17647% with 33 lines in your changes missing coverage. Please review.

Project coverage is 32.42%. Comparing base (d212bb1) to head (aa64f48). Report is 10 commits behind head on main.

Files	Patch %	Lines
...rm/services/popup-view-cache-background.service.ts	33.33%	14 Missing :warning:
...orm/popup/view-cache/popup-router-cache.service.ts	81.63%	6 Missing and 3 partials :warning:
apps/browser/src/background/main.background.ts	0.00%	4 Missing :warning:
apps/browser/src/popup/app.component.ts	0.00%	2 Missing :warning:
...t-wrapper/extension-anon-layout-wrapper.stories.ts	0.00%	1 Missing :warning:
...rc/platform/popup/layout/popup-header.component.ts	75.00%	1 Missing :warning:
.../src/platform/popup/layout/popup-layout.stories.ts	0.00%	1 Missing :warning:
apps/browser/src/popup/app-routing.module.ts	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9556      +/-   ##
==========================================
+ Coverage   32.38%   32.42%   +0.03%     
==========================================
  Files        2654     2658       +4     
  Lines       80876    80995     +119     
  Branches    15241    15260      +19     
==========================================
+ Hits        26193    26263      +70     
- Misses      52668    52713      +45     
- Partials     2015     2019       +4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

Checkmarx One – Scan Summary & Details – 574ad0a0-fe1a-4898-b21c-d6eb9412e949

Fixed Issues

Severity	Issue	Source File / Package
	Unpinned Actions Full Length Commit SHA	/publish-desktop.yml: 115
	Unpinned Actions Full Length Commit SHA	/publish-web.yml: 44
	Unpinned Actions Full Length Commit SHA	/publish-cli.yml: 171
	Unpinned Actions Full Length Commit SHA	/publish-cli.yml: 129
	Unpinned Actions Full Length Commit SHA	/publish-desktop.yml: 124
	Unpinned Actions Full Length Commit SHA	/publish-cli.yml: 92

[willmartian]

Hold until 64e6bab is removed.

edit: done

[vleague2]

I think I would prefer to accept only primitive types in the cache, such as id strings that can then be mapped in the code back to the objects they represent. This feels more like the way I'd expect to interact with a database -- store and receive only a reference key for an object, and then build mapping utilities to match that to the correct object in the code. Then we can sidestep the issues of object equality and remove the changes to the chip select and select. I think it would be more maintainable (i.e. we wouldn't have to keep making changes to other components or code that eventually become involved with the caches) and it's not an unreasonable amount of work for devs to map to and from the cache objects.

[nick-livefront]

@shane-melton Mentioned this PR after discussing PM-9455 with me.

After pulling down these changes and reverting my previous solution, this would indeed solve PM-9455. I'll create a PR stacked on top of this one to revert my changes once this one lands.

[willmartian]

@audreyality ty for the review and sorry for the delay responding!

❌ Seems like this could leak arbitrary secrets from a view into plaintext global state.

In what way? Values are save to memory and aggressively cleared at process reload, logout, account switch, lock, browser tab change, Angular route change, and 2 minutes after the popup was closed.

🤔 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.

Why?

• The driver of the feature is: "okay, restoring the last page we were on is easy enough. But what if the user changed stuff without saving?"
• My design here is inspired by the idea of essentially replaying the user input. That is why one of the core entry points to caching data is through a form group.

🎨 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.

For this and the prior point, what is a specific example of a place where you might need deeper control over when the value is cleared? Or where the security and consistency requirements differ from the general implementation of this PR?

📝 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);

I intentionally wanted to expose a signal over an observable here:

• Angular is moving in a direction where where in-component reactivity is expressed in signals
• Signals also support arbitrary mapping with computed
  • const bar = computed(() => someOtherSignal() + 1
• Signals can be converted to/from RxJS Observables if needed.

However, I don't even expect cacheSignal to be used as often as cacheForm. It simply felt right to expose both since cacheForm composes cacheSignal

[audreyality]

In what way?

I was wrong about this. The last time I looked in-depth at storage locations large object storage didn't exist. I wasn't aware that it uses session storage that does not persist to disk.

🤔 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.

Why?

Because the service is responsible for making business decisions which the UI code shouldn't know about. Consider: you stated your design replays the user input. I can see two ways this can fail.

• Nondeterminism due to product requirements: That requires that a service is deterministic. Any service that generates cryptographic material shouldn't be generating it deterministically. (Encrypt/decrypt is deterministic. I'm talking about the entropy source.)
• Nondeterminism due to reactivity: Reactive systems are nondeterministic by nature. It's important that the "settle" into a state over time, but eventual consistency is "baked in" to the design pattern. The UI inputs are deterministic, but the timing of an external event (item sync, crosstalk due to storage sharing, chrome or IPC events, OS events) will not be deterministic. Services have access to all of these signals, however, and thus they can determine when it's safe to store temporary state and keep track of what invalidates it.

For this and the prior point, what is a specific example of a place where you might need deeper control over when the value is cleared?

I unfortunately can't provide a specific example, as I only know a very small part of the overall system. I'm basing this on the overall patterns I've seen when solving the "save temporary state" problem in software that met the algorithmic determinism requirements by design. Even though the algorithms were deterministic and we did all of the state management using systems specialized for each kind of state, we were still solving these problems 3 years later.

State management is a hard problem in distributed systems, and bitwarden is way more distributed than what I'd built. Sorry that all I can offer is spideysense and vibes. This is the kind of thing that you don't realize is expensive to maintain until you're trapped in a sunk cost fallacy.

However, I don't even expect cacheSignal to be used as often as cacheForm. It simply felt right to expose both since cacheForm composes cacheSignal.

Now I get to sound like Oscar 😉.

My problem here is that there are so many ways to solve the same problem (callbacks, promises, rxjs, signals, message queues, ports, channels...), and of them all signals are the least well understood. Now, add to this that we just finished converting to rxjs, everyone is still learning reactive principles, and that most systems (that I've seen, at least) don't coordinate their data flows. That all says to me that we should refine rxjs before exposing new ways to manage reactivity.

Just as far as it goes: I agree with the outcome you're trying to enable, and I'm also excited about singals. In a personal project, I'd just use them (well, I'd use Vue, which has a similar reactivity model). In a project the scale of bitwarden, though, I go through the architecture team when introducing new concepts.

So far, I have yet to convince them that any of my modernization ideas (most of which are older and better understood than signals) are worth the overhead of learning the new concept. They're not being antagonistic--I just haven't had the time to circle back. I know reactivity quite well, but I'm still coming up to speed on our current suite of tech.

So, in the meantime, I'm extending their concepts. Introducing things like the buffered state provider (which is really nice) and the secret state provider (which is interesting, but not the right abstraction in hindsight). That's what I'm advocating here.

That all said, the fact the code's already been written in all likelihood means it'll merge. So I'm going to stop here. If you want to continue talking about this, let's do it in a call. :)

[willmartian]

I am splitting the route history and view cache functionality into two PRs (route history in this pr; view cache here) because I plan to chat with Audrey more about the above and the route history portion of this seems like it needs less discussion because it won't be surfaced to other teams. 🫡

(Honestly, it should have been like this from the get go--live and learn!)

[willmartian]

1822294 and 65b9ff5 fixes tests that were failing since the popup header component now has a dependency on GlobalStateProvider.

[willmartian]

aa64f48 fixes type errors resulting from code in libs/vault importing code from apps/browser that surfaced here due to changes to the popup header component. @shane-melton confirmed these imports were included by mistake, so I removed them.