Skip to content

feat: notification pagination#183

Merged
chris13524 merged 17 commits intomainfrom
feat/notification-pagination-read
Mar 5, 2024
Merged

feat: notification pagination#183
chris13524 merged 17 commits intomainfrom
feat/notification-pagination-read

Conversation

@chris13524
Copy link
Member

@chris13524 chris13524 commented Dec 7, 2023

Requirements

This PR adds new methods to the authentication page, rather than splitting these out on the RPC methods page since this is easier to understand in one spot. A subsequent PR will consolidate both pages into one.

  • Adds missing id field from notification
  • Adds methods for getting a page of notifications, which clients should call on start just like watchSubscriptions and use to overwrite local state
  • Adds method for getting a specific notification by ID
  • Adds method for marking a notification as read
  • Adds method for receiving updates when a notification changes (i.e. read status changing)
  • Adds method for counting how many unread notifications there are

Remaining work:

  • Add to RPC methods
  • Dapp key vs notify key, and topic?
  • Update client SDKs
  • Flag to getNotifications to order unread notifications first
  • Create docs PR with updates & migration guide and share with beta testers for feedback

Open questions:

  • Fix: Get by ID: URL doesn't include this ID
    • Solution: getByUrl method or getByNotificationId method (the notification_id the app provided when sending the notification)?
  • Do we need a flag to only paginate over e.g. read or unread notifications, or a way to put read notifications first in the response?
    • No, there is not currently a desire to show unread notifications first, and getUnreadNotificationCount solves the need of counting these.
  • Assumption right now is we will remove delete methods from client SDKs, and we do not need an archived or deleted state. Is read/unread sufficient?
    • No current desire I am aware of
  • Should unread notification count be part of watchSubscriptions instead of its own method in order to improve the performance of app.web3inbox.com? Or do we need a batch method?

@chris13524 chris13524 self-assigned this Dec 7, 2023
@vercel
Copy link

vercel bot commented Dec 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
walletconnect-specs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 5, 2024 7:51pm

Copy link
Member Author

@chris13524 chris13524 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's break this into 3 PRs with the first to be prioritized:

  • Get notification history/pagination & removal of delete notifications
  • Read/unread state, notification change events, unread notification count
  • Get notification by ID

@chris13524 chris13524 changed the title feat: notification pagination & read state feat: notification pagination Jan 10, 2024
@chris13524
Copy link
Member Author

This PR has been updated to only include the details required for notification history/pagination which has been successfully implemented. Remaining details have been broken out into separate PRs for future discussion:

Copy link
Contributor

@devceline devceline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chris13524 chris13524 merged commit 1bf5ba5 into main Mar 5, 2024
@chris13524 chris13524 deleted the feat/notification-pagination-read branch March 5, 2024 19:23
bkrem pushed a commit that referenced this pull request May 23, 2024
chris13524 added a commit that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants