Code Review

[marcelklehr]

Hey @MB-Finski

I had a look at the code you wrote for this and am listing things that could be improved here as an overview :)

Backend

• I think it would be good to introduce PHP-Code-Sniffer, so that the repo uses the canonical Nextcloud coding style. The corresponding CI action is already installed, the only thing it now needs is a composer.json file containing the command and the dependency
• The CI workflows also contain a REUSE workflow that is currently failing. It would be good to fix the issues mentioned in the action run here
• The CI workflow for linting PHP is also failing because of a missing composer.json command (see https://github.com/nextcloud/integration_openai/blob/main/composer.json where you've done this already)
• Currently the github actions workflow folder also contains workflows for phpunit, which currently don't do anything. As long as no tests are implemented we can leave these out, I think: https://github.com/nextcloud/gptfreeprompt/blob/main/.github/workflows/phpunit-sqlite.yml
• The getTitle() method of the reference provider currently always returns the english title. Would be nice to translate that as in the comment https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Reference/GptFreePromptReferenceProvider.php#L39
• To avoid confusion, I recommend not reusing QueryBuilder objects and creating new ones for each new query (e.g. here https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Db/PromptMapper.php#L179 )
• PromptMapper#createPrompt: The way the mapper classes are usually used is to create the model object (ie. Prompt) outside of the mapper class and then pass it to update/insert methods. You can then also update the timestamp by overriding the insert/update methods setting the new timestamp on the model and then calling parent::insert($prompt) https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Db/PromptMapper.php#L115-L138
• In your PromptMapper you have a query that sorts by timestamp. It would then make sense to add an index for userId,timestamp to make sure this query performs well: https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Migration/Version010000Date20230929162000.php#L51
• GptFreePromptService#processPrompt: you are calling runTask. This works well for e.g. openai providers but will fail with e.g. the llm app because it will take longer than the max execution time of php (usually 30-60s). I recommend using runOrSchedule (which hopefully makes it into 28) https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Service/GptFreePromptService.php#L39

Frontend

• Currently the picker seems to only be displayed when the user is an admin? https://github.com/nextcloud/gptfreeprompt/blob/main/src/reference.js#L15
• The frontend looks really nice, though!

Other

• Usually we check in screenshots into the repository as well to have them be part of the licensing and allow people to collaborate on them https://github.com/nextcloud/gptfreeprompt/blob/main/appinfo/info.xml#L19C109-L19C112

Other than these the app looks already quite nice! Nice job!
Marcel

[MB-Finski]

Thanks for the review! Everything should be done now that I got the last few changes necessary for runOrSchedule done. We originally planned with Julien to make the integration synchronous so that's why implementing runOrSchedule required quite a lot of changes (notifications, UI rewrite, backend changes). But it's finally done and I think this should help us have quite a bit better user experience with the llm app.

I elected to keep createPrompt as-is since it aligns with the implementations in integration_openai (from which it was derived) and text2image_helper. I'll probably change it if I end up rewriting the db entities/mappers when moving the UI items under the assistant app.

Also, php unit tests are already in the works so I left the workflows in place.

[marcelklehr]

Hi Sami,

thanks for your efforts to implement these changes. I see you did a lot of refactoring. Great job!

I have a few remarks still. See below :)

createPrompt

I elected to keep createPrompt as-is since it aligns with the implementations in integration_openai (from which it was derived) and text2image_helper. I'll probably change it if I end up rewriting the db entities/mappers when moving the UI items under the assistant app.

Wise choice to keep compatibility in that case. 👍

phpunit tests

php unit tests are already in the works so I left the workflows in place.

Woop woop! 🎉

CI

• I realized most CI workflows are not running because they only listen for changes on the master or stable branches, but in this repo the branch is called main: https://github.com/nextcloud/gptfreeprompt/blob/main/.github/workflows/lint-php.yml#L14-L15 After fixing that it would be good if those lint actions would be green like the christmas tree :)

processPrompt

https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Service/GptFreePromptService.php#L103

• I think this doesn't do what you intended it to do :D The | operator is a binary or. You are effectively asking if the status is equal to 2|1, which is 3. So, you are asking if the status is STATUS_SUCCESSFUL.

https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Service/GptFreePromptService.php#L111

• This line is superfluous, I think. The later call to insert along with the try-catch should be enough.

notify

https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Listener/GptFreePromptResultListener.php#L83

Reading this I wonder, will this notify the user 10 times if they asked for 10 responses?

frontend

Currently the picker seems to only be displayed when the user is an admin?

Not sure what I was smoking when I wrote that... :D Sorry

[MB-Finski]

Thanks for the fast response!

* [x]  I think this doesn't do what you intended it to do :D The `|` operator is a binary or. You are effectively asking if the status is equal to `2|1`, which is `3`. So, you are asking if the status is STATUS_SUCCESSFUL.

Haha! I was wondering if it were some odd php syntax when you wrote me that I can just test if "status === FAILED|SUCCESS" when we discussed the text2image api. I took your message too literally. Funnily enough, the reason the code kind of works is that the equality is tested first so (x === 1 | 2) will always be non-zero and the if() will thus execute.

I should've double checked since I had my doubts knowing that in most languages that'd be a binary operator.

[marcelklehr]

Ah, damn, my bad, sorry for being too cryptic :D

[MB-Finski]

I think I should have the new comments now mostly covered. I'm not sure why the xml lint fails, though.

notify

https://github.com/nextcloud/gptfreeprompt/blob/main/lib/Listener/GptFreePromptResultListener.php#L83

Reading this I wonder, will this notify the user 10 times if they asked for 10 responses?

Good that you commented on this! You mark a notification as processed by creating a notification with the same id object and userId equal to those of an existing notification and then you mark that notification instance as processed. With this in mind I was assuming that creating a new notification with the same id object and userId as an existing one would overwrite the old notification... This would've given a quite nice user experience since the user would be re-notified if a new generation was completed after dismissing the earlier notification. But curiously enough, now that I tested this, the old notification is NOT overwritten even when the id object and userId already exist (since the timestamp is different, I assume). It would be REALLY nice if the notifications had proper docs.

[marcelklehr]

It would be REALLY nice if the notifications had proper docs.

I feel your pain :/