Skip to content
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

🐛 Bug Report: Headless Client Page Based Pagination Error #5053

Closed
2 tasks done
lwuethrich-devedis opened this issue Jan 7, 2024 · 11 comments
Closed
2 tasks done
Labels

Comments

@lwuethrich-devedis
Copy link

📜 Description

This problem affects the headless client and might affect the prebuilt notification center components as well.

Currently when fetching user notification using HeadlessService#fetchNotifications page based pagination is used with
a page number and an optional limit.
This returns notifications for a user with notifications sorted by createdAt date descending, so newer notifications are first in list.

However, when new notifications are created this page number has now a wrong offset as the newer notifications are inserted in front of th existing ones. This means fetching the next page will return previously fetched notifications as well.

A cursor based pagination would solve this problem as this is not dependent on the order of elements or if elements are added or deleted. This could be both for the oldest notification fetched as well as for fetching newly created notifications.

👟 Reproduction steps

  1. Fetch notifications in the headless client (HeadlessService#fetchNotifications) with page set to 0
  2. Insert a new notifications
  3. Fetch again with page set to 1

👍 Expected behavior

It should return the next notifications given the page and limit correctly.

👎 Actual Behavior with Screenshots

It returns previously fetched notifications as the page number is not correct anymore.

Novu version

Novu SaaS

npm version

No response

node version

No response

📃 Provide any additional context for the Bug.

No response

👀 Have you spent some time to check if this bug has been raised before?

  • I checked and didn't find a similar issue

🏢 Have you read the Contributing Guidelines?

Are you willing to submit PR?

None

Copy link

linear bot commented Jan 7, 2024

@github-actions github-actions bot added the triage label Jan 7, 2024
@Vijaykv5
Copy link

Vijaykv5 commented Jan 7, 2024

I would love to work on this issue. Please assign me

@lwuethrich-devedis
Copy link
Author

@Vijaykv5 who are you talking to? I do not have permissions to assign someone to a ticket.

@Vijaykv5
Copy link

Vijaykv5 commented Jan 9, 2024

I'm talking to maintainers mate :)

@vichustephen
Copy link
Contributor

Hi there , I believe this issue is fixed. I mean when a new notification is received in your case the API will refetch the data .
You can check with the issue here : https://github.com/novuhq/novu/issues/5011

Copy link
Contributor

scopsy commented Jan 22, 2024

Please let me know if #5011 fixed the issue, closing for now 🙏

@scopsy scopsy closed this as not planned Won't fix, can't repro, duplicate, stale Jan 22, 2024
@lwuethrich-devedis
Copy link
Author

@scopsy Sry for replying late. The issue seems to persist. I tested it with the following method:

I crate a headless client and fetch the first page. In between I trigger new notifications. When I fetch the next page the same notification is returned. I haven't looked at the internals of the headless client yet but the issue seems to be that page based pagination is used instead of cursor based pagination. The seem issue will also be present when using the server api.

curl --location --request POST 'https://api.novu.co/v1/events/trigger' \
     --header 'Authorization: ApiKey <your-api-key>' \
     --header 'Content-Type: application/json' \
     --data-raw '{
         "name": "test-workflow",
         "to": {
           "subscriberId": "testId"
         },
         "payload": {}
       }'
import { IPaginatedResponse, IMessage } from "@novu/headless";

const headlessService = new HeadlessService({
  applicationIdentifier: "_V7h_cvx9VVb",
  subscriberId: "testId",
});

headlessService.initializeSession({
  listener: (res: FetchResult<ISession>) => {
  },
  onSuccess: (session: ISession) => {
    const waitTime = 10000;
    fetchNotfications(0);
    new Promise(r => setTimeout(r, waitTime)).then(() => {
      fetchNotfications(1);
    });
    new Promise(r => setTimeout(r, waitTime)).then(() => {
      fetchNotfications(2);
    });

  },
  onError: (error) => {
  },
});


function fetchNotfications(page: number) {

  headlessService.fetchNotifications({
    listener: ({ data, error, isError, isFetching, isLoading, status }) => {
    },
    onSuccess: (response: IPaginatedResponse<IMessage>) => {
      console.log(
        JSON.stringify(response.data.map((it) => it._id)),
      );
    },
    page: page, // page number to be fetched
    query: {
      limit: 1
    }
  });
}```

Result: I my first run I do not send a notification between fetching and I get three different messageIds back.
In the second run I send notifications in between fetching the next page and I get the same notification back.

test-novu > npx ts-node main.ts
["65b0b24c594c10c96a5bb190"]
["65b0b0f9594c10c96a5b3330"]
["65b0b103aee218b06659cd95"]

test-novu > npx ts-node main.ts
["65b0b24c594c10c96a5bb190"]
["65b0b24c594c10c96a5bb190"]
["65b0b51ad690bce686743ac0"]

@lwuethrich-devedis
Copy link
Author

@vichustephen Sorry for replying quite late I had some things to take care of. See my previous comment, the issue seems to persist.

@lwuethrich-devedis
Copy link
Author

@vichustephen @scopsy I believe this issue still has to be reopened

@vichustephen
Copy link
Contributor

Hi @lwuethrich-devedis please let me know client library version that you are using. Are you trying out with the latest version ?

@lwuethrich-devedis
Copy link
Author

@vichustephen With the latest version of novu/headless 0.24.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants