feat: Implement interface persistence #2856
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2856 +/- ##
=======================================
Coverage 94.46% 94.47%
=======================================
Files 486 486
Lines 10360 10391 +31
Branches 1582 1590 +8
=======================================
+ Hits 9787 9817 +30
- Misses 573 574 +1 ☔ View full report in Codecov by Sentry. |
| */ | ||
| #validateContentType(contentType: string) { | ||
| if (!(contentType in ContentType)) { | ||
| throw new Error('Invalid content type.'); |
| * @param id - The interface id. | ||
| * @param contentType - The type of content. | ||
| */ | ||
| updateInterfaceContentType(id: string, contentType: ContentType) { |
There was a problem hiding this comment.
Wondering if this is overkill for now. Since we strictly need this for static notifications, this adds a lot of complexity that isn't currently needed (assuming notifications aren't created using createInterface). WDYT?
There was a problem hiding this comment.
It's required to reliably mark an interface's content type. At various points in the system we accept interface id's instead of the JSX content itself. This is meant to address those situations. I didn't want to expose the concept of contentType to the end user and thus chose not to try and force it in the createInterface method.
There was a problem hiding this comment.
Agreed that we shouldn't allow the user to modify this. My comment was more regarding that we only need this information for notifications right now, so we don't need to solve it for all other types of interfaces yet (if at all).
There was a problem hiding this comment.
Ohh, that's valid. Hmm I'll make some changes.
| @@ -132,7 +140,22 @@ export class SnapInterfaceController extends BaseController< | |||
| super({ | |||
| messenger, | |||
| metadata: { | |||
| interfaces: { persist: false, anonymous: false }, | |||
| interfaces: { | |||
| persist: (interfaces: Record<string, StoredInterface>) => { | |||
There was a problem hiding this comment.
Wondering if we should have any limits on this? How long do we persist Snap notifications for?
There was a problem hiding this comment.
So this is mainly to cover the edge case where the interface is lost for the detailed view notification across sessions. I realize it would persist an unread notification's interface indefinitely if the user never reads it, but that's an extreme edge case. I've started conversation with the notification team about exposing some settings to the user to control the lifecycle of notifications.
| Record<string, StoredInterface> | ||
| >((persistedInterfaces, [id, snapInterface]) => { | ||
| switch (snapInterface.contentType) { | ||
| case ContentType.Notification: |
There was a problem hiding this comment.
How are notification interfaces ever deleted?
There was a problem hiding this comment.
Interface cleanup has to be done in the extension (at the end of the notification timeout). We don't have access to the controller messenger in the constructor so can't grab the notifications themselves and see what's expired from here.
There was a problem hiding this comment.
Hmm, for other interfaces we do this in the extension but that is because we have no other choice. It may make sense to listen for an event or something similar from the NotificationController to delete the persisted interfaces instead? 🤔
There was a problem hiding this comment.
Oooo I like that, yeah I can do that.
Implementing interface persistence based on a new property stored in the interface object called
contentType, it is used to indicate whether theSnapInterfaceControllershould persist the interface. The property is added at various points in our system where an interface is created.