Save chatroom logs in KV store #1998
Conversation
Passing run #2349 ↗︎
Details:
Review all test suite changes for PR #1998 ↗︎ |
|||||||||||||||
| signingKeyId: sbp('chelonia/contract/currentKeyIdByName', ourIdentityContractId, 'csk') | ||
| }) | ||
| }, | ||
| 'gi.actions/identity/resetChatRoomLogs': () => { | ||
| return sbp('okTurtles.eventQueue/queueEvent', CHATROOM_LOGS, async () => { | ||
| const currentChatRoomLogs = await sbp('gi.actions/identity/loadChatRoomLogs') | ||
| sbp('state/vuex/commit', 'setChatRoomLogs', currentChatRoomLogs) | ||
| }) | ||
| }, | ||
| 'gi.actions/identity/addChatRoomLog': (contractID: string) => { | ||
| return sbp('okTurtles.eventQueue/queueEvent', CHATROOM_LOGS, () => { | ||
| return requireToSuccess(async () => { | ||
| const currentChatRoomLogs = await sbp('gi.actions/identity/loadChatRoomLogs') | ||
|
|
||
| if (!currentChatRoomLogs[contractID]) { | ||
| currentChatRoomLogs[contractID] = { readUntil: null, unreadMessages: [] } | ||
| await sbp('gi.actions/identity/saveChatRoomLogs', contractID, currentChatRoomLogs) | ||
| } | ||
| }) | ||
| }) | ||
| }, | ||
| 'gi.actions/identity/setChatRoomReadUntil': ({ contractID, messageHash, createdHeight }: { | ||
| contractID: string, messageHash: string, createdHeight: number | ||
| }) => { | ||
| return sbp('okTurtles.eventQueue/queueEvent', CHATROOM_LOGS, () => { | ||
| return requireToSuccess(async () => { | ||
| const currentChatRoomLogs = await sbp('gi.actions/identity/loadChatRoomLogs') | ||
|
|
||
| const exReadUntil = currentChatRoomLogs[contractID].readUntil | ||
| if (exReadUntil === null || exReadUntil.createdHeight < createdHeight) { | ||
| const exUnreadMessages = currentChatRoomLogs[contractID]?.unreadMessages || [] | ||
| currentChatRoomLogs[contractID] = { | ||
| readUntil: { messageHash, createdHeight }, | ||
| unreadMessages: exUnreadMessages.filter(msg => msg.createdHeight > createdHeight) | ||
| } | ||
|
|
||
| await sbp('gi.actions/identity/saveChatRoomLogs', contractID, currentChatRoomLogs) | ||
| } | ||
| }) | ||
| }) | ||
| }, | ||
| 'gi.actions/identity/deleteChatRoomReadUntil': ({ contractID, deletedHeight }: { | ||
| contractID: string, deletedHeight: number | ||
| }) => { | ||
| return sbp('okTurtles.eventQueue/queueEvent', CHATROOM_LOGS, () => { | ||
| return requireToSuccess(async () => { | ||
| const currentChatRoomLogs = await sbp('gi.actions/identity/loadChatRoomLogs') | ||
|
|
||
| if (currentChatRoomLogs[contractID].readUntil.deletedHeight === undefined) { | ||
| currentChatRoomLogs[contractID].readUntil['deletedHeight'] = deletedHeight | ||
| await sbp('gi.actions/identity/saveChatRoomLogs', contractID, currentChatRoomLogs) | ||
| } | ||
| }) | ||
| }) | ||
| }, | ||
| 'gi.actions/identity/addChatRoomUnreadMessage': ({ contractID, messageHash, type, createdHeight }: { | ||
| contractID: string, messageHash: string, type: string, createdHeight: number | ||
| }) => { | ||
| return sbp('okTurtles.eventQueue/queueEvent', CHATROOM_LOGS, () => { | ||
| return requireToSuccess(async () => { | ||
| const currentChatRoomLogs = await sbp('gi.actions/identity/loadChatRoomLogs') | ||
|
|
||
| // NOTE: should ignore to add unreadMessages before joining chatroom | ||
| if (currentChatRoomLogs[contractID].readUntil) { | ||
| const index = currentChatRoomLogs[contractID].unreadMessages.findIndex(msg => msg.messageHash === messageHash) | ||
| if (index < 0) { | ||
| currentChatRoomLogs[contractID].unreadMessages.push({ messageHash, type, createdHeight }) | ||
| await sbp('gi.actions/identity/saveChatRoomLogs', contractID, currentChatRoomLogs) | ||
| } | ||
| } | ||
| }) | ||
| }) | ||
| }, | ||
| 'gi.actions/identity/deleteChatRoomUnreadMessage': ({ contractID, messageHash }: { | ||
| contractID: string, messageHash: string | ||
| }) => { | ||
| return sbp('okTurtles.eventQueue/queueEvent', CHATROOM_LOGS, () => { | ||
| return requireToSuccess(async () => { | ||
| const currentChatRoomLogs = await sbp('gi.actions/identity/loadChatRoomLogs') | ||
|
|
||
| // NOTE: should ignore to delete unreadMessages before joining chatroom | ||
| if (currentChatRoomLogs[contractID].readUntil) { | ||
| const index = currentChatRoomLogs[contractID].unreadMessages.findIndex(msg => msg.messageHash !== messageHash) | ||
| if (index >= 0) { | ||
| currentChatRoomLogs[contractID].unreadMessages.splice(index, 1) | ||
| await sbp('gi.actions/identity/saveChatRoomLogs', contractID, currentChatRoomLogs) | ||
| } | ||
| } | ||
| }) | ||
| }) | ||
| }, | ||
| 'gi.actions/identity/deleteChatRoomLog': ({ contractID }: { contractID: string }) => { | ||
| return sbp('okTurtles.eventQueue/queueEvent', CHATROOM_LOGS, () => { | ||
| return requireToSuccess(async () => { | ||
| const currentChatRoomLogs = await sbp('gi.actions/identity/loadChatRoomLogs') | ||
| delete currentChatRoomLogs[contractID] | ||
| await sbp('gi.actions/identity/saveChatRoomLogs', contractID, currentChatRoomLogs) | ||
| }) | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Could you please explain what these functions are for? This seems like it's storing a lot of data and I thought the issue was about just how the unread message location is stored.
Would it be possible to store the unread message information just by storing the hash of the oldest unread message?
There was a problem hiding this comment.
It looks like there are many utility functions to manage chatroom logs. But I don't think it's not that too much, and they are necessary. I will explain the functions one by one.
loadChatRoomLogs and saveChatRoomLogs
These two functions are to load/save chatRoomLogs from/to the backend.
resetChatRoomLogs
This function is to load chatRoomLogs from the backend and set vuex state with it.
addChatRoomLog and deleteChatRoomLog
These functions are to create(or initialise)/delete logs for one chatroom inside the chatRoomLogs. As I have mentioned before, the chatRoomLogs is in the following format.
chatRoomLogs: {
[chatRoomID]: {
readUntil: { messageHash, createdHeight, deletedHeight? },
unreadMessages: [{ messageHash, type, createdHeight }]
}
}
So in initialising, just create a new log for the new chatroom, and set it { readUntil: null, unreadMessages: [] }
setChatRoomReadUntil and deleteChatRoomReadUntil
These two functions are to set/delete the readUntil state inside the chatRoomLog.
addChatRoomUnreadMessage and deleteChatRoomUnreadMessage
These two functions are to set/delete the unreadMessages state inside the chatRoomLog.
So actually, I can say that we save two data in chatRoomLogs for each chatRoom. Those are readUntil and unreadMessages.
- The
readUntilis used to set theisNewindicator position, and sometimes to set the initial scroll position. I thinkisNewindicator position should be same throughout all the devices for the same user, and it should be saved in KV store. - The
unreadMessagesis used to save the new messages of different types;MESSAGE_TYPES.TEXT,MESSAGE_TYPES.INTERACTIVE, andMESSAGE_TYPES.POLL. It also used to calculate the number of unread messages which would be set the value of the badge for the chatroom.
You asked me if we should simplify the code by saving only unreadMessages which is a list of only message hashes.
- Even though, we would save only
unreadMessages, I think the utility functions would have the same logic, and only two functions would be removed;setChatRoomReadUntilanddeleteChatRoomReadUntil. But I think we should also save thereadUntilin order to keep the sameisNewindicator position throughout all the devices andreadUntilis also used to deleteunreadMessages. - If we save only message hash in the
unreadMessages, there would be problem to get to know their message type, and to delete some/all of them when user scrolls down and sees the messages.
So, in my honest opinion, I think all the functions are necessary, and the attributes of chatRoomLogs too.
Additionally, what I am going to say is that all the chatRoomLogs utility functions should be in a same invocation queue. That's because they could be run at the same(not exactly same, but with a few difference) time, and could set the wrong data unless they are in the same invocation queue.
There was a problem hiding this comment.
Thanks for that rundown @Silver-IT. I need to study your code more to come to a more informed conclusion.
However, I have some more questions:
You asked me if we should simplify the code by saving only
unreadMessageswhich is a list of only message hashes.
Sorry for not being clear, I actually meant what about saving only readUntil? I.e. only saving the hash of the message that we didn't read, and maybe (if necessary) saving a counter of the number of unread messages?
It seems like this code is saving unread messages in two different places:
- In the KV store
- In the contract Vuex state
It would be awesome if we could just save it in one place (2), the contract Vuex state.
There was a problem hiding this comment.
Sorry for not being clear, I actually meant what about saving only
readUntil? I.e. only saving the hash of the message that we didn't read, and maybe (if necessary) saving a counter of the number of unread messages?
I think there would be confusion from the names; readUntil, and unreadMessages.
Actually, unreadMessages doesn't store all the messages received after readUntil message. Just like in Slack, the red badge of the channel doesn't mean the number of all the unread messages. It is the number of mentions received after the last-seen message. unreadMessages stores only that kind of messages (The reason of that I can't call it kind of mentions is that we currently store three types of messages; TEXT, POLL, INTERACTIVE). And readUntil is the last-seen message and is used to display isNew indicator. So readUntil and unreadMessages are all needed, in my opinion.
It would be awesome if we could just save it in one place (2), the contract Vuex state.
Yeah, we save chatRoomLogs in two places. Permanently in KV store, and temporarily in Vuex store. I guess you can understand why we should save them in KV store. The reasons why I think we need to save chatRoomLogs in Vuex store too are that we access the chatRoomLogs many times inside the Chat page and that I don't think we should access the KV store every time we need access. And also the another reason is that we have many vuex getters.
In the previous version, we did also save in two places; indexedStorage and Vuex store. The current version is similar but just exchanged indexedStorage with KV store.
There was a problem hiding this comment.
@Silver-IT I'm still not understanding. Why are you storing messages in the KV Store?
unreadMessages stores only that kind of messages
Why?
The current version is similar but just exchanged indexedStorage with KV store.
Why?
Can you not store messages in the KV Store?
The Vuex store should have only the ~20 or so most recent messages. The rest should be fetched using the eventsAfter API. And I see no reason to store messages in the KV Store... and so far no clear explanation for it
There was a problem hiding this comment.
OK, thanks for helping clarify some things, but it's still not clear to me why you are storing the message hashes.
The only thing that's needed to display a red ------------------ unread messages line in the UI, plus the count of the unread messages, is the hash of the oldest unread message, plus a count of the number of unread messages.
That's it. So is it possible for you to store just that information, and nothing else?
There was a problem hiding this comment.
@taoeffect, I updated this PR and simplified the chatRoomLogs format by deleting deletedHeight of readUntil, type of unreadMessages. And also deleted the utility function 'gi.actions/identity/deleteChatRoomReadUntil'.
// BEFORE
chatRoomLogs: {
[chatRoomID]: {
readUntil: { messageHash, createdHeight, deletedHeight? },
unreadMessages: [{ messageHash, type, createdHeight }]
}
}
// AFTER
chatRoomLogs: {
[chatRoomID]: {
readUntil: { messageHash, createdHeight },
unreadMessages: [{ messageHash, createdHeight }]
}
}
- About
readUntil- Why is
messageHashneeded? In order to load events until thatreadUntil.messageHashwhen user opens the chat page. - Why is
createdHeightneeded? In order to set theisNewindicator position by comparing the heights of messsages to thisreadUntil.createdHeight.
- Why is
- About
unreadMessages- Why is
unreadMessagesarray? Why not save only the number of unread messages? We can not save only the number of unread messages, because we need the following two fields. - Why is
messageHashneeded? Let's think about this case. user1 sends messageHi @user2in the chatroom and deletes that message. For user2, that message should be added to theunreadMessagesarray and also needs to be deleted again.Without themessageHash(and not saving that message hash inunreadMessagesarray or by only saving number of unread messages), we can not decide what to do with it. Should delete something from the list? Or should reduce the number of unread messages? Can not decide. - Why is
createdHeightneeded? Let's think about this case. user1 sends many messages with mentions to user2. Yeah, really many message of several pages. When user2 opens the chat page, what should he see? It's the last scroll position if it's the same device, or thereadUntilmessage if it's the another device. At this moment, user2 haven't seen all the mentions because there are many messages of several pages. If the user2 scrolls down and checks messages one by one, the number of unread messages should be reduced one by one by comparing the heights of messages to the heights of unread messages.
- Why is
If we really need to reduce the KV store, and the chatRoomLogs too, I have a solution. In the above chatRoomLogs format, we really need createdHeight. And we need messageHash either in the current version of project but it's not 100% necessary to save messageHash if we update the workflow. Now, we use messageHash in many places; to save chatRoomScrollPosition, to create message link, ... (Not sure for more)
If we replace them with the message height, we can also remove messageHash in the chatRoomLogs.
Here, what we need to consider is that based on the @corrideat idea, many messages could be created in one event. Many messages with different message hashes could have same height.
@taoeffect, if you agree on my suggestion to replace messageHash to messageHeight in all chatroom related data, I will create an issue and work on it soon. I think it could need some much changes so I would like to work it in another issue.
There was a problem hiding this comment.
Thank you for the explanation @Silver-IT.
@taoeffect, if you agree on my suggestion to replace messageHash to messageHeight in all chatroom related data, I will create an issue and work on it soon.
That's OK, I think we can leave it as-is.
I will continue with the review now. One more note though (and I will add a comment on this elsewhere in the review), but I think chatRoomLogs needs to be renamed to something like chatroomUnreadMessages in order to be clearer and more accurate.
There was a problem hiding this comment.
Yeah, I will rename chatRoomLogs to chatRoomUnreadMessages.
However, it would have a field of unreadMessages, I think we should find a better name?
chatRoomUnreadMessages: {
[chatRoomID]: {
readUntil: { messageHash, createdHeight },
unreadMessages: [{ messageHash, createdHeight }]
}
}
There was a problem hiding this comment.
However, it would have a field of
unreadMessages, I think we should find a better name?
That seems fine to me, they are unread messages 🤷♂️
| const requireToSuccess = async (fnToSuccess) => { | ||
| if (typeof fnToSuccess !== 'function') { | ||
| return | ||
| } | ||
|
|
||
| let failureCount = 0 | ||
| do { | ||
| try { | ||
| return await fnToSuccess() | ||
| } catch (err) { | ||
| console.log('[requireToSuccess:]', err) | ||
| failureCount++ | ||
| } | ||
| } while (failureCount < 10) | ||
| } |
There was a problem hiding this comment.
I think @taoeffect or @corrideat could recommend the better way to handle this.
There was a problem hiding this comment.
What is the reason for this function? It seems unnecessary to me. What happens if we remove it?
There was a problem hiding this comment.
What is the reason for this function?
This is similar to how we do when publish events. As you know, if it fails to publish an event, we should try again and again to make it succeed. Similar to the above case, it could fail to save the value to KV store sometimes. So, we retry to make sure that the value to be saved in KV store.
What happens if we remove it?
It's same to skip an event not to be published. It's the solution of the issue which @corrideat and I discussed here.
frontend/controller/actions/group.js
Outdated
| @@ -958,7 +958,7 @@ export default (sbp('sbp/selectors/register', { | |||
|
|
|||
| // Wait for any pending operations (e.g., sync) to finish | |||
| await sbp('chelonia/queueInvocation', contractID, async () => { | |||
| const current = await sbp('chelonia/kv/get', contractID, 'lastLoggedIn')?.data || {} | |||
| const current = (await sbp('chelonia/kv/get', contractID, 'lastLoggedIn'))?.data || {} | |||
There was a problem hiding this comment.
This is to fix the Issue #1997.
| const requireToSuccess = async (fnToSuccess) => { | ||
| if (typeof fnToSuccess !== 'function') { | ||
| return | ||
| } | ||
|
|
||
| let failureCount = 0 | ||
| do { | ||
| try { | ||
| return await fnToSuccess() | ||
| } catch (err) { | ||
| console.error('[requireToSuccess:]', err) | ||
| failureCount++ | ||
| } | ||
| } while (failureCount < 10) | ||
| } | ||
|
|
There was a problem hiding this comment.
@Silver-IT This function can now be removed once #2027 is merged into this PR. Could you please have a look at that PR and merge it when ready?
| sideEffect ({ contractID, hash, meta }) { | ||
| setReadUntilWhileJoining({ contractID, hash, createdDate: meta.createdDate }) |
There was a problem hiding this comment.
As the name of function shows, it's for setting readUntil position of a chatroom.
In the previous way of using Indexed Storage, we set the readUntil position to the latest message when the user syncs the chatroom contract for the first time. So, every function that creates a message had this function call in it's sideEffect to set the readUntil to the message which is created in it's process.
But, in the current way of using KV store, readUntil position is initialised only in chatroom/join/sideEffect function, and can be updated only when the messages are displayed in the screen and user sees them. So, the updating of readUntil can't be processed in the background and it's handled in ChatMain.vue.
| if (!this.searchText && !this.selections.length) { | ||
| return this.ourRecentConversations | ||
| } | ||
| return this.ourRecentConversations.filter(({ title, partners }) => { | ||
| const partnerIDs = partners.map(p => p.contractID) | ||
| const upperCasedSearchText = String(this.searchText).toUpperCase().normalize() | ||
| if (!difference(partners, this.selections).length) { | ||
| if (!difference(partnerIDs, this.selections).length) { | ||
| // match with contractIDs | ||
| return false | ||
| } else if (String(title).toUpperCase().normalize().indexOf(upperCasedSearchText) > -1) { | ||
| return true | ||
| } else if (String(partners.join(', ')).toUpperCase().indexOf(upperCasedSearchText) > -1) { | ||
| } else if (String(title).toUpperCase().normalize().includes(upperCasedSearchText)) { | ||
| // match with title | ||
| return true | ||
| } else { | ||
| // match with username and displayname | ||
| const userKeywords = upperCasedSearchText.replace(/\s/g, '').split(',') | ||
| return userKeywords.reduce((found, userKeyword, index, arr) => { | ||
| const isLastUserKeyword = index === arr.length - 1 | ||
| let currentFound = false | ||
| if (isLastUserKeyword) { | ||
| currentFound = partners.findIndex(p => { | ||
| return p.username.toUpperCase().normalize().includes(userKeyword) || | ||
| p.displayName.toUpperCase().normalize().includes(userKeyword) | ||
| }) >= 0 | ||
| } else { | ||
| currentFound = partners.findIndex(p => { | ||
| return p.username.toUpperCase().normalize() === userKeyword || | ||
| p.displayName.toUpperCase().normalize() === userKeyword | ||
| }) >= 0 | ||
| } | ||
| return found && currentFound | ||
| }, true) | ||
| } | ||
| return false | ||
| }) | ||
| }).sort((a, b) => a.partners.length > b.partners.length ? 1 : -1) |
There was a problem hiding this comment.
What are these changes for, and which issue are they related to?
There was a problem hiding this comment.
What are these changes for, and which issue are they related to?
This change is for the third stuff of this PR summary.
Improved to search DMs by keyword (Possible to use usernames, and displayNames)
There was a problem hiding this comment.
While I was testing the searching DMs, I noticed that in case I tried to search DMs by keyword, I can't get the DMs that I wanted to search.
Example cases;
Let's imagine that we have four users; alexjin(Silver-IT), greg(Taoeffect), sebin(Sebin Song), andrea
And also we have three DMs; Taoeffect, Sebin Song, Taoeffect, andrea, Sebin Song
- We couldn't find any DMs using the keyword
gregsince the keywordgregdoesn't exist in the title of any DMs - We couldn't find any DMs using the keyword
andrea, Taoeffectsince the keywordandrea, Taoeffectdoesn't exist in the title of any DMs - When we type keyword
Sebin Song, it displays two DMs;Sebin Song, Taoeffect, andreaandSebin Songin order. And when I press Enter, the first DM was opened. But I thought the second DM(Sebin Song) would be opened because I only typed keywordSebin Song
There was a problem hiding this comment.
Great work @Silver-IT! I did some testing and found an issue. Not sure if it's this PR or something else, but we should fix it:
Here's what I did in Firefox Dev Edition with container tabs (note: not all of these steps might be necessary):
- [Tab1] u1 creates group, creates
#randomchannel, copies link - [Tab2] u2 joins group
- [Tab1] u1 sends DM to u2
- [Tab2] u2 sends DMs back to u1, switches to
#generalchannel - [Tab1] u1 sends more DMs to u1
- [NEW PRIVATE WINDOW CREATED] login as u2
There was a problem hiding this comment.
Excellent work @Silver-IT! This was a difficult PR and you did a great job!!
Summary of Changes