-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Summarize email thread #8653
Conversation
Set to draft because we'll do the full MVP on this branch |
FYI before testing nextcloud/server#39658 |
Text processing APIs only exist with 27.1, 28 and later. This app supports older releases. So we need to find a way to hide this feature when the API is not there. The AiIntegrationsService could inject ContainerInterface instead and then do something like try {
$manager = $this->container->get(IManager::class);
$manager->magic();
} catch (ContainerError $e) {
// hide feature
} |
Not blocking but very nice to have: unit tests for all new classes |
6ca9536
to
4bc481e
Compare
I can create an issue and add them in a separate PR? so this gets released asap |
Any advice on how to make psalm happy for older versions? |
Extend Lines 22 to 41 in ab14bd6
|
7e1f262
to
c38d8a2
Compare
$id, | ||
$messages, | ||
$this->currentUserId, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling this in a route is risky as it will likely exceed the PHP max execution time. Also, you might want to cache the output. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works. However, there are some issues in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested without any llm backend
Ideally we hide the whole feature when there is no text summarization support. That is hide the admin settings and the summary box above threads.
3412c1b
to
6528bad
Compare
I found myself in a funny situation. I have thread summaries enabled since yesterday. But there is no UI to disable them. Nextcloud users could run into this as well if they previously had an llm integration enabled but turned it off. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting small changes one last time
Solid PR 👍 😎
src/service/SettingsService.js
Outdated
} | ||
|
||
export const isLlmConfigured = async () => { | ||
const url = generateUrl('/apps/mail/api/settings/llmconfigured') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can save one request at page load if this retrieval is moved into initial state
@@ -244,6 +244,11 @@ public function index(): TemplateResponse { | |||
$this->config->getAppValue('mail', 'allow_new_mail_accounts', 'yes') === 'yes' | |||
); | |||
|
|||
$this->initialStateService->provideInitialState( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always set summaries to no
if llm apis are not available
Signed-off-by: hamza221 <[email protected]>
1857c59
to
0cb4ed5
Compare
The necessary parts in the server are not yet released. You have to be a bit more patient (or install a beta at your own risk). |
To Test the setting should be enabled first in admin settings
closes #8508