Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: handle assistant sync task #5269

Merged
merged 3 commits into from
Feb 28, 2024
Merged

feat: handle assistant sync task #5269

merged 3 commits into from
Feb 28, 2024

Conversation

luka-nextcloud
Copy link
Contributor

📝 Summary

  • update assistant name space
  • add insert button for assistant sync task result
  • use notifications for triggering fetch instead of polling

🖼️ Screenshots

🏚️ Before 🏡 After
B A

🚧 TODO

  • ...

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@luka-nextcloud luka-nextcloud added bug Something isn't working 3. to review labels Jan 16, 2024
@luka-nextcloud luka-nextcloud self-assigned this Jan 16, 2024
@juliusknorr juliusknorr added enhancement New feature or request and removed bug Something isn't working labels Jan 22, 2024
@juliusknorr juliusknorr mentioned this pull request Jan 22, 2024
Merged
juliusknorr
juliusknorr reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comments, otherwise this looks good and works (with nextcloud/server#43005 for testing)

src/components/Assistant.vue Outdated
@@ -213,7 +214,7 @@ export default {
},
computed: {
showAssistant() {
return !this.$isRichWorkspace && !this.$isPublic && window?.OCA?.TPAssistant?.openAssistantForm
return !this.$isRichWorkspace && !this.$isPublic && window?.OCA?.TpAssistant?.openAssistantForm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julien-nc Was there a casing change in the API here? Is it bound to a Nextcloud release?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, last release has it upper case and main branch as well (just some intermediate state before nextcloud/assistant#27 had the p lower case) so @luka-nextcloud We should revert this change to be TPAssistant

Copy link
Member

@julien-nc julien-nc Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late answer. This was and will stay OCA.TPAssistant.

@juliusknorr
Copy link
Member

@luka-nextcloud Could you also take the time to add cypress coverage for this part? We can enable the testing app for this in the github action.

@juliusknorr
Copy link
Member

Tested and works function wise 👍

@julien-nc I noticed some odd behaviour with nextcloud/server#43005 where having multiple text processing providers enabled leads to the async one alwas being used in the assistant modal. I haven't touched any configuration, though the AI settings show the synchronous one as a default:

Screenshot 2024-01-23 at 19 48 22

@luka-nextcloud luka-nextcloud force-pushed the bugfix/assistant-sync-task-handle branch from 6915ba4 to cd4d726 Compare January 24, 2024 18:14
@luka-nextcloud
Copy link
Contributor Author

@luka-nextcloud Could you also take the time to add cypress coverage for this part? We can enable the testing app for this in the github action.

I've added it, please check.

juliusknorr
juliusknorr reviewed Jan 24, 2024
View reviewed changes
cypress/entrypoint.sh
@@ -19,8 +19,10 @@ if [ $? -eq 0 ]; then
php /var/www/html/occ user:add --password-from-env user2
php /var/www/html/occ app:enable viewer
php /var/www/html/occ app:enable text
php /var/www/html/occ app:enable assistant
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was for some way of running isolated cypress tests locally, but never used that script myself. @max-nextcloud Do you happen to know if we still make use of that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember it. Looking at the log of that file indicates that it was introduced by @susnux in f62c29b.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just for local tests, but nowadays we could use @nextcloud/cypress/docker which also works locally :)

@luka-nextcloud luka-nextcloud force-pushed the bugfix/assistant-sync-task-handle branch from cd4d726 to ec0a368 Compare January 25, 2024 08:56
@luka-nextcloud luka-nextcloud force-pushed the bugfix/assistant-sync-task-handle branch from ec0a368 to 750084a Compare January 29, 2024 06:11
@juliusknorr juliusknorr force-pushed the bugfix/assistant-sync-task-handle branch from 750084a to 4eccbde Compare February 20, 2024 09:45
@juliusknorr
Copy link
Member

@luka-nextcloud Could you check why the cypress tests fail there?

@julien-nc
Copy link
Member

I noticed some odd behaviour with nextcloud/server#43005 where having multiple text processing providers enabled leads to the async one alwas being used in the assistant modal. I haven't touched any configuration, though the AI settings show the synchronous one as a default:

@juliushaertl Thanks, I'll check that. Maybe there's a mismatch between the default value displayed in the settings frontend and the actual default selected provider when creating a task.

@luka-nextcloud luka-nextcloud force-pushed the bugfix/assistant-sync-task-handle branch from 4eccbde to 10ebf2e Compare February 27, 2024 07:51
@juliusknorr
Copy link
Member

Pushed a small adjustment for the github action that should make cypress pass.

@juliusknorr juliusknorr merged commit 65bc514 into main Feb 28, 2024
60 checks passed
@juliusknorr juliusknorr deleted the bugfix/assistant-sync-task-handle branch February 28, 2024 18:28
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
@julien-nc julien-nc mentioned this pull request Mar 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien-nc julien-nc julien-nc left review comments

@susnux susnux susnux left review comments

@juliusknorr juliusknorr juliusknorr approved these changes

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud max-nextcloud is a code owner

Assignees

@luka-nextcloud luka-nextcloud

Labels
3. to review enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assistant synchronous task handling and polishing
5 participants
@luka-nextcloud @juliusknorr @julien-nc @susnux @max-nextcloud