Conversation
jaller94
left a comment
jaller94
left a comment
There was a problem hiding this comment.
Happy to see history support in MUCs being worked on. 👏
Here are some minor notes, because you asked for a quick review.
|
We should only send room history when the room's history visibility settings allow users to see history. What is the best way to get the room's history settings? |
jaller94
left a comment
jaller94
left a comment
There was a problem hiding this comment.
The HistoryManager class would be a good one to have tests for.
In general, I think this PR is already pretty good.
Please remove the Draft state if you're looking for an Approve or Request changes review.
| */ | ||
| interface IHistoryStorage { | ||
| // add a message to the history of a given room | ||
| addMessage: (chatName: string, message: Element, jid: JID) => unknown; |
There was a problem hiding this comment.
Why allow unknown to be returned and not just void?
There was a problem hiding this comment.
Because it could be async and return Promise<void>
| // TODO: make a class that stores in PostgreSQL so that we don't lose history | ||
| // when we restart |
There was a problem hiding this comment.
Do you want to file that as an issue or address this before merging this PR?
There was a problem hiding this comment.
Probably just file as an issue.
Co-authored-by: Christian Paul <christianp@matrix.org>
Half-Shot
left a comment
Half-Shot
left a comment
There was a problem hiding this comment.
This looks worthy of a go to me
Half-Shot
left a comment
Half-Shot
left a comment
There was a problem hiding this comment.
Just occurred to me that we should respect history vis.
|
hi, can this be merged now? it seems the requested changes have been resolved and the reviewer removed themselves. This would mitigate the downsides of missing #64 |
5 participants
No description provided.