Add codeblock rendering support using monaco for AI messages #2172
Conversation
…ering support using monaco for AI messages
| if (this.isNewRoom(named.roomId)) { | ||
| this.resetCache(); | ||
| } |
There was a problem hiding this comment.
this is not a good strategy. it defeats the purpose of the cache and its a really awkward way to constrain the values returned from this reource to only those that pertain to this room. A better approach is to keep this cache, but then filter out only items that pertain to this room in our various getters, and to make sure those getters use a @cached decorator
| get messages() { | ||
| return [...this._messageCache.values()].sort( | ||
| (a, b) => a.created.getTime() - b.created.getTime(), | ||
| ); | ||
| if (this.roomId == undefined) { | ||
| return []; | ||
| } | ||
| return [...this._messageCache.values()] | ||
| .filter((m) => m.roomId === this.roomId) | ||
| .sort((a, b) => a.created.getTime() - b.created.getTime()); | ||
| } | ||
|
|
||
| @cached | ||
| get members() { | ||
| return Array.from(this._memberCache.values()) ?? []; | ||
| if (this.roomId == undefined) { | ||
| return []; | ||
| } | ||
| return ( | ||
| Array.from(this._memberCache.values()).filter( | ||
| (m) => m.roomId === this.roomId, | ||
| ) ?? [] | ||
| ); | ||
| } | ||
|
|
||
| @cached | ||
| get invitedMembers() { | ||
| return this.members.filter((m) => m.membership === 'invite'); | ||
| if (this.roomId == undefined) { | ||
| return []; | ||
| } | ||
| return this.members.filter( | ||
| (m) => m.membership === 'invite' && m.roomId === this.roomId, | ||
| ); | ||
| } | ||
|
|
||
| @cached | ||
| get joinedMembers() { | ||
| return this.members.filter((m) => m.membership === 'join'); | ||
| if (this.roomId == undefined) { | ||
| return []; | ||
| } | ||
| return this.members.filter( | ||
| (m) => m.membership === 'join' && m.roomId === this.roomId, | ||
| ); |
There was a problem hiding this comment.
note that these caches span rooms. that is fine, it makes for quick access when switching between rooms, but it means that we should filter these values by room and use a @cached decorator so we only do this filtering when the room switches--not for every access of the property
| // we pass the code thru using localstorage instead of in the DOM, | ||
| // that way we don't have to worry about escaping code. note that the | ||
| // DOM wants to render "<template>" strings when we put them in the | ||
| // DOM even when wrapped by <pre>. also consider a codeblock that has | ||
| // a "</pre>" string in it. |
There was a problem hiding this comment.
this strategy actually worked out really well. so our AI bot's messages can have any manner of code within them., however we are not using this approach for messages that originate from the user, and those do have code escaping problems. like if a user tries to type <template> in a message, that message will be rendered without the string<template>. you can use your imagination for other things that might be problematic. I suspect that the HTML sanitization may be getting in our way here. users should be able to ask questions about code. filing this as a bug
…rove-rendering-code-blocks-emitted-by-ai-model
| 'class', | ||
| 'preview-code monaco-container code-block', | ||
| ); | ||
| monacoContainer.setAttribute('style', `height: ${lines + 4}rem`); |
There was a problem hiding this comment.
This is a simple heuristic to get the codeblock to have a variable height based on the content. The monaco div itself does not automaticlaly grow based on the contents within it. (note that there is a min-height/max-height CSS prop on this to make sure this is well behaved)
| } | ||
| // note that since the localstorage item was set from runtime-common there | ||
| // was no ember-window-mock available, so we can't use window mock here | ||
| let maybeContent = window.localStorage.getItem(id); |
There was a problem hiding this comment.
It’s worrisome to me that this custom lint rule isn’t triggering 😳
| .dom('button.code-copy-button') | ||
| .exists('the copy code to clipboard button exists'); | ||
|
|
||
| // assert that new messages don't destabilize the RoomMessage component |
There was a problem hiding this comment.
What actually asserts this?
There was a problem hiding this comment.
Adding a new event would previously destabilize the RoomMessage component (as described in the PR description). The copy button and Monaco editors are from a modifier that adds DOM. A rerender of this component would blow away the DOM added by their modifier. Meaning that the assertions below this added event would fail.
Yeah I added assertions that will fail (you commented on it actually) if this starts happening again. The CodeBlockModifier adds DOM to the RoomMessage. If it rerenders that added DOM from the modifier will get blown away. |
ah great, I wasn’t grasping that, thanks for the explanation 🤩 |
|
I noticed a visual annoyance when the assistant is streaming file contents to the user. When the code is streamed, it looks like the code editor is continuously rerendering which produces flashing (check the video below). Probably this is out of scope for this PR and we should probably make a separate ticket: code-flashing.mov |
i think the alternative would be to show it in a |
|
@jurgenwerk I updated the PR in the latest commit so that we don't render the code block while the ai bot response is streaming. |
…rove-rendering-code-blocks-emitted-by-ai-model
This PR adds codeblock rendering support using monaco for AI messages. As part of this there was a lot of cleanup that i had to do for
RoomResourceandRoomMessageComponent. Specifically there were a couple problems:RoomMessageComponenton the screen was re-rendering for every matrix event received, regardless of whether the matrix event had anything to do with theRoomMessageComponent. This was due to the fact that theRoomMessageComponentuse an array ofMessageobjects as one of it's args. The thing is that for every access ofRoomResource.messagethe array was rebuilt (it was not cached), moreover, even when it is cached, it's still creating a brand new array for each new message received in a room. We need to stabilize ourRoomMessageComponenton theRoomResourceso that it is not rebuilding on every single matrix event.RoomResourcehas caches that span rooms. This is fine--as you jump between your rooms it's nice to use cached values. However, the cache was blown away every time you switched rooms, and the way this was done was a bit dubious. The better solution here is to retain the caches of items that span rooms and in the various getters filter out the pertinent items from the rooms, and moreover to use a@cacheddecorator to prevent the filter from running on every consumption of the property, and only recomputed when the room changes.I believe that our message scrolling logic is actually dependent on the
RoomMessageComponentre-render bug. so very likely we'll need to fix this again. (/cc @FadhlanR)