-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Implement interface persistence #2856
base: main
Are you sure you want to change the base?
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.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be tested
* @param id - The interface id. | ||
* @param contentType - The type of content. | ||
*/ | ||
updateInterfaceContentType(id: string, contentType: ContentType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should have any limits on this? How long do we persist Snap notifications for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are notification interfaces ever deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 🤔
Implementing interface persistence based on a new property stored in the interface object called
contentType
, it is used to indicate whether theSnapInterfaceController
should persist the interface. The property is added at various points in our system where an interface is created.