Add mention detection to update_message #302
Conversation
This patch extends the mention detection functionality from add_message (added in PR jupyterlab#235) to update_message. This enables automatic @mention detection when messages are updated, which is particularly useful for streaming scenarios where message content is built incrementally. Changes: - Refactored mention detection logic into reusable _extract_mentions() helper method - Updated add_message to use the new helper method - Added mention detection to update_message that scans the complete message body - Added three test cases covering mention updates, append with mentions, and deduplication - Fixed regex pattern to use raw string to avoid SyntaxWarning The implementation ensures no duplicate mentions are added when the same message is updated multiple times, making it safe for streaming use cases.
|
|
Zsailer
requested review from
brichet and
dlqqq
and removed request for
dlqqq
|
Looks great @Abigayle-Mercer! Just left a minor comment about the argument name. |
There was a problem hiding this comment.
Thanks @Abigayle-Mercer for working on this.
I have some comments below.
| updated_msg = chat.get_message(msg_id) | ||
| assert updated_msg | ||
| # Should only have one mention despite appearing twice | ||
| assert set(updated_msg.mentions) == set([USER2.username]) |
There was a problem hiding this comment.
AFAIK the set function returns unique elements, which would no prove that USER2 is not mentioned several times.
| return uid | ||
|
|
||
| def update_message(self, message: Message, append: bool = False): | ||
| def update_message(self, message: Message, append: bool = False, find_mentions: bool = False): |
There was a problem hiding this comment.
IIUC the expectation is to find the mentions for the last update of a streaming message.
Maybe in future we'd like to trigger other actions on the last update. I wonder if we should rename this parameter last_update or something similar to be more generic.
I also wonder if the default value should be True, to trigger the find_mentions by default when this is not a streaming message.
| def update_message(self, message: Message, append: bool = False, find_mentions: bool = False): | |
| def update_message(self, message: Message, append: bool = False, last_update: bool = True): |
What do you think?
There was a problem hiding this comment.
I just realized that it has been discussed and updated in another comment, with the opposite argument 😄
I still think that the default could be True, though, but don't have a strong opinion on it.
There was a problem hiding this comment.
😆 I think you make a great point though, @brichet.
Maybe this should be more generic... like trigger_actions—where actions are a set of callbacks that get triggered when the message is updated. Right now, we can just hardcode a single action, finding mentions. This would allows us to expand that API to include other actions without changing this method signature.
There was a problem hiding this comment.
I still think that the default could be
True
I wrestled with this a bit too. I think it should be False. The simple argument, is that keeps backwards compatibility. Right now, streaming doesn't ever parse the mentions, so this won't introduce unexpected behavior.
But the deeper reason is that if the developer is not explicit when using this API, it has the potential to trigger a bunch of actions repeatedly, unintendedly. While it's a bit more verbose in the downstream code, I think it's best to force folks to be explicit when triggering actions.
4 participants
This patch extends the mention detection functionality from
add_message(added in PR #235) toupdate_message, enabling automatic @mention detection when messages are updated. This is particularly useful for streaming scenarios where message content is built incrementally, allowing personas to mention each other while streaming.Changes:
_extract_mentions()helper methodadd_messageto use the new helper methodfind_mentionsparameter for streaming controlfind_mentionsparameterStreaming Support:
The new
find_mentionsparameter allows callers to control when mention extraction and notifications occur:update_message(chunk, append=True, find_mentions=False)- defers mention extractionupdate_message(message, find_mentions=True)- triggers mention extraction and persona notificationsThis ensures personas are only notified once when the complete message is ready, rather than on every incremental update. The implementation ensures no duplicate mentions are added when messages are updated multiple times, making it safe for streaming use cases.
Integration:
This PR enables a forthcoming PR #11 to
stream_message()in BasePersona (jupyter-ai-persona-manager) that will leverage thefind_mentionsparameter to properly handle @mentions in streaming responses. The companion PR will add a finalupdate_message(..., find_mentions=True)call after streaming completes, ensuring mentioned personas are notified only when the full message is available.