-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[FIX] Messages was grouping wrong some times when server is slow #10472
Conversation
5ef2b0f
to
c1ee40a
Compare
You da man! That's been haunting us for a while now 😁 |
@@ -223,7 +223,7 @@ const RoomManager = new function() { | |||
}; | |||
|
|||
const loadMissedMessages = function(rid) { | |||
const lastMessage = ChatMessage.findOne({rid}, {sort: {ts: -1}, limit: 1}); | |||
const lastMessage = ChatMessage.findOne({rid, temp: { $exists: false } }, {sort: {ts: -1}, limit: 1}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing!
@@ -339,7 +339,19 @@ Template.message.onViewRendered = function(context) { | |||
return this._domrange.onAttached(function(domRange) { | |||
const currentNode = domRange.lastNode(); | |||
const currentDataset = currentNode.dataset; | |||
const previousNode = currentNode.previousElementSibling; | |||
const getPreviousSentMessage = (currentNode) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My biggest concern here is performance. If we are creating a new function for every message, then I can imagine the memory level just went up a little bit more...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, can we have some monitoring on unstable and develop when this hits? @geekgonecrazy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@graywolf336 do you think simply moving the function to outer scope would help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure about that, as I don't know if it would be created everytime the template is initialized. That would be something to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how garbage collector works for these lambda functions used/created on closed contexts.. if that is a real problem or not..
what I know is moving it to file's root scope will define it once and will not run every time the template is initialized.
Closes #9926
Tested locally with @geekgonecrazy's script for load testing and this seems to fix most of the issues.
Reproducing while offline (Messages on the right are grouped with the wrong user)
After fix:
Even though the timestamp is not respected, the messages are grouped correctly.