Refactor loadMessage function in room resource.
#2047
Conversation
FadhlanR
changed the title
loadMessage function in room resource.
FadhlanR
requested review from
lukemelia,
backspace,
burieberry,
IanCal,
habdelra,
jurgenwerk,
tintinthong and
richardhjtan
and removed request for
lukemelia and
backspace
| @@ -1313,7 +1313,7 @@ export default class MatrixService extends Service { | |||
| ? JSON.parse(event.content.data) | |||
| : event.content.data | |||
| ) as CommandResultWithOutputContent['data']; | |||
| this.ensureCardFragmentsLoaded(data.cardEventId, roomData); | |||
| await this.ensureCardFragmentsLoaded(data.cardEventId, roomData); | |||
There was a problem hiding this comment.
Sometimes there is an error says "no cardFragments found". And I noticed that we forgot to add await to this line. It caused card message event are able to be processed without card fragment event.
There was a problem hiding this comment.
nice catch, I wish there were a way to lint against this but I gather it’s not that simple
| @@ -1313,7 +1313,7 @@ export default class MatrixService extends Service { | |||
| ? JSON.parse(event.content.data) | |||
| : event.content.data | |||
| ) as CommandResultWithOutputContent['data']; | |||
| this.ensureCardFragmentsLoaded(data.cardEventId, roomData); | |||
| await this.ensureCardFragmentsLoaded(data.cardEventId, roomData); | |||
There was a problem hiding this comment.
nice catch, I wish there were a way to lint against this but I gather it’s not that simple
| } | ||
|
|
||
| export function formattedMessageForCommand(formattedBody: string) { | ||
| return `<p data-test-command-message class="command-message">${formattedBody}</p>`; |
There was a problem hiding this comment.
I know this is just copied from a different position but does anyone know if we have XSS protection or the like for this? Or is it a safe assumption that malicious messages of this event type can’t be produced?
There was a problem hiding this comment.
I just added this 43de05a.
| @@ -81,7 +81,7 @@ export default class Room extends Component<Signature> { | |||
| @registerConversationScroller={{this.registerConversationScroller}} | |||
| @setScrollPosition={{this.setScrollPosition}} | |||
| > | |||
| {{#each this.messages as |message i|}} | |||
| {{#each this.messages key='eventId' as |message i|}} | |||
Problem
I initially investigated the scrolling position issue in the AI panel, where it would jump back to the top whenever new messages were added or updated. The root cause was that we were reinstantiating the messages in the room resource every time a new event occurred. This caused Ember to rerender all the messages in the room, preventing the browser from maintaining the scrolling position.
Solution
In this PR, I refactored the loadMessage function to avoid reinstantiating messages when new events occur. Instead, the event now updates only the necessary parts of the message.
Bugs that are fixed with this PR: