fix(contextchat): use a consistent item id for indexed messages - #13443
fix(contextchat): use a consistent item id for indexed messages#13443kesselb wants to merge 1 commit into
Conversation
|
/backport to stable5.10 |
Content was indexed as "{mailboxId}:{messageId}" but deleted by bare
IMAP uid, so deleting a message never removed it from the knowledge
base. Key both sides off account, mailbox and uid via a shared helper.
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
# Conflicts:
# lib/ContextChat/ContextChatProvider.php
d190fbe to
7fadd20
Compare
|
/backport to stable5.11 |
edward-ly
left a comment
edward-ly
left a comment
There was a problem hiding this comment.
Also roping in @marcelklehr for additional insight. I'll test this later if I have time, but it looks good to me so far. Do we need to do some sort of migration as well (e.g. updating the IDs of existing items, or deleting and re-importing all messages again)?
@kyteinsky and I did some brainstorming about it (in helpdesk). It would be indeed good to reset the index. The best option for now seems to change the provider id. My first idea was to trigger deleteProvider with a migration, but Anupam explain it runs async and hence we cannot really know when it's done. By using a different provider id deleting and re-importing can run in parallel. |
Follow up |
| } catch (\Throwable) { | ||
| // Context chat fails the whole answer if this throws |
There was a problem hiding this comment.
thanks for pointing this out, fixed in nextcloud/context_chat#266
| // Context chat calls this when it renders the sources of an answer, so the | ||
| // message id is looked up on demand rather than baked into the item id. | ||
| try { | ||
| $mailbox = $this->mailboxMapper->findById((int)$mailboxId); | ||
| $messageId = $this->messageMapper->getIdForUid($mailbox, (int)$uid); |
There was a problem hiding this comment.
getItemUrl is called for every item in the sources list when the output is rendered, true
but without this change, it was still not looked up until the link was clicked by the user. Here, every link would be checked even when not clicked.
not sure what's the difference between uid and id but the with id approach, the message was only checked when the user clicked on it and the webpage from 'mail.page.thread' opened.
I'm not familiar with the mail's code so sorry if the point does not make much sense.
There was a problem hiding this comment.
Thanks a lot, that's a valid concern. The previous version was a cheap call.
id = oc_mail_messages.id
uid = identifier on the imap server, unique per mailbox, also stored as oc_mail_messages.uid
Better would be the id, but some of the events (e.g. MessageDeletedEvent) does not carry it.
But that might change very soon, and hence I will move this PR back to draft for now and continue once the JMAP changes are in. At best we have id everywhere, otherwise I will add a new entrypoint that accepts mailboxId/uid doing the lookup internally.
4 participants
Content was indexed as "{mailboxId}:{messageId}" but deleted by bare IMAP uid, so deleting a message never removed it from the knowledge base. Key both sides off account, mailbox and uid via a shared helper.
馃 AI (if applicable)