prose-im / prose-core-client

Prose core XMPP client manager & protocols.
https://prose.org
Mozilla Public License 2.0
23 stars 3 forks source link

Support MAM pagination #58

Closed nesium closed 9 months ago

nesium commented 9 months ago

Provide methods to not only load the latest (batch of) messages but also earlier ones.

Refs #22 Refs prose-im/prose-app-web#55

valeriansaliou commented 9 months ago

Could that one be made a priority over other issues? I’d like to get things right with message history in the app.

nesium commented 9 months ago

Sure thing!

nesium commented 9 months ago

New API:

export class ArchiveID {}

export class Message {
  // …
  readonly archiveId: ArchiveID | undefined;
  // …
}

export class MessageResultSet {
  /// Is `true` when the `MessageResultSet` contains messages from the last (earliest by date) page.
  readonly isLast: boolean;
  /// The requested messages in the order from oldest to newest.
  readonly messages: Message[];
}

export interface RoomBase {
  // …
  loadLatestMessages(): Promise<MessageResultSet>;
  loadMessagesBefore(before: ArchiveId): Promise<MessageResultSet>;
  // …
}

I've also created a public channel pagination-test that contains 500 messages at intervals of 2 seconds.

Also loadLatestMessages() and loadMessagesBefore() now loads 100 messages instead of 50 from MAM.

Note:

valeriansaliou commented 9 months ago

Thanks Marc!

On the limit of 100 messages vs reactions, I believe the core should abstract this so that the Prose app gets a guarantee that it receives 100 (ie. N) messages during paging, meaning that the core should internally continue paging in the same async/await block until it reaches N returned messages (or reaches the start of the archive).

Some safety limits should of course apply there, eg. to prevent a reaction DOS from triggering very slow loads on client's ends. I am however unsure of what we should do in the case where the page before would only contain reactions, meaning it would return 0 entries.

If we keep on paging internally and there are only reaction stanzas (ie. following a reaction DOS attack), and hit the safety limit, would that mean that the app (ie. the Prose Web app for instance) would try loading with the same before ArchiveId value when the user would scroll up again to try loading the previous page once again, and getting zero results? Should 0 results indicate that the end of the archive has been reached?

nesium commented 9 months ago

I was thinking the same thing Re: DOS attack. I could put the ArchiveId into the MessageResultSet, that way you could still load try to load the next page even if we hit the safety limit. I don't know how you would trigger it again UI-wise though, since you're already at the top of the message list and shouldn't be the one to trigger the DOS attack either.

valeriansaliou commented 9 months ago

That'd work yes. If the result set returned is empty but the ArchiveID is not empty then I'd consider we're not done paging, and use that one to load the page before. If it's void/null then I'll assume we're done paging (or use isLast).

valeriansaliou commented 9 months ago

Also, could you maybe add a optional parameter to specify how many messages to load in both methods? That'll help w/ debugging an implementation since I could pass a RSM size of eg. 10 and test paging, since 100 is quite large to test everything is working as expected (although it's a good default).

valeriansaliou commented 9 months ago

And, last one, if I deref isLast from loadMessagesBefore result I get Error: null pointer passed to rust.

valeriansaliou commented 9 months ago

Updates:

nesium commented 9 months ago

Alright, MessageResultSet now looks like this…

export class MessageResultSet {
/**
* Can be used to load more messages. `lastMessageId` might not be contained in `messages`.
* If not set there are no more messages to load.
*/
  readonly lastMessageId: ArchiveID | undefined;
/**
* The requested messages in the order from oldest to newest.
*/
  readonly messages: Message[];
}

Unless there are no more messages on the server, messages is now guaranteed to contain at least 100 messages or more. It contains more than 100 messages if we needed to load another batch to fill the 100. In that case I'm not throwing away the difference.

valeriansaliou commented 9 months ago

Thanks, all implemented in https://github.com/prose-im/prose-app-web/commit/15fae64245722684c18555a1006f10846a30b3cd

nesium commented 9 months ago

Closing this in favor of #59