Repository navigation
Conversation
Fixes nextcloud#695 Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 36 minutes. View limit details
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 7 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main change: offering notifications when a chat reply is ready. Description check The description is related to the changeset because it identifies the tracked issue and the required browser notification background-click fix. Linked Issues check The changes satisfy the coding requirements in #695.InputAreashows a dismissible banner after 12 seconds and provides a per-reply notification toggle.ChattyLLMInputFormtracks active and resume…Out of Scope Changes check The changes remain within #695. Backend notification handling supports per-reply success and failure delivery and conversation navigation. Frontend changes implement the banner, delayed display, permi…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 20.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 7 files. (2 skipped: 2 unsupported.)
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
I think the added UI element should be more discreet. On a system that will execute the task in a short time, it feels weird to see this big note card appear.
Maybe only adding a button on the right, next to the submit/audio one, would be enough. We would loose the "you can close the page" explanation text but we would keep the "click this if you want to be notified when the assistant produced a response" as the HTML title of the button.
Wdyt?
The prominent banner is deliberate, it follows how Claude does it: the opt-in is one shot and easy to miss when it hides behind a tooltip, and the "you can close the page" part is half of what the issue (Fu) asks for, imo. A bare button with a title attribute would drop that entirely. But I would agree with the "weird on a quick action" part (from what I assume you mean), so I'd lean towards gating the visibility conditionally. Only issue on my part is a possible scope creep, doing it "properly" would involve I'd be up to gating it behind a set 10-15s timer, if that helps. Achieves a comparable compromise with less coupling. wdyt @julien-nc |
Sounds good. But then, one cannot immediately enable notification for a task after submitting the message. Fair enough. The note card label could be improved IMO. |
Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
I'd keep the current label though: it's one line by design, the longer variant wraps and doubles the card height, and the Notify action next to it is self-explanatory. Timer gate it is, will push it shortly. The tradeoff after submitting is fine by me. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/Notification/NotifierTest.php (1)
99-150: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tests for
successandfailurewith a nulltarget.The current tests cover only chat subjects. A test with
target => nullforsuccesswould expose the issue at Line 74 ofNotifier.php.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
32a9facd-ba9c-4965-a2fc-0902bd98cf50
📒 Files selected for processing (10)
lib/Listener/TaskFailedListener.phplib/Listener/TaskSuccessfulListener.phplib/Notification/Notifier.phplib/Service/NotificationService.phpsrc/components/AssistantTextProcessingForm.vuesrc/components/ChattyLLM/ChattyLLMInputForm.vuesrc/components/ChattyLLM/InputArea.vuesrc/utils.jstests/unit/Notification/NotifierTest.phptests/unit/Service/NotificationServiceTest.php
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Require the background-push click fix for chat notifications. · Notifier.php:413-455
lib/Notification/Notifier.php:413-455
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire the background-push click fix for chat notifications.
NotificationService::sendNotification()emitschat_successandchat_failurefor chat tasks.Notifierassigns the target to both the notification link and theIAction::TYPE_WEBaction. When the page is closed, opening that target depends on the Nextcloud notifications background-push click handler.Nextcloud 35 is supported, but its stable-35 backport of fix
#3478is not merged. A supported deployment can therefore deliver the notification while failing to open the chat. Require a notifications release containing#3478, or add an equivalent local fallback before relying on this behavior.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
0cc06753-dd46-4bef-8730-510b293d2f74
📒 Files selected for processing (4)
lib/Notification/Notifier.phpsrc/components/ChattyLLM/ChattyLLMInputForm.vuesrc/components/ChattyLLM/InputArea.vuetests/unit/Notification/NotifierTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/Notification/NotifierTest.php
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Background (closed-tab) notification clicks fully resolve once nextcloud/notifications#3478 ships |
| thinkingText: t('assistant', 'Processing…'), | ||
| scheduledText: t('assistant', 'Waiting…'), | ||
| submitBtnAriaText: t('assistant', 'Submit'), | ||
| notifyBannerText: t('assistant', 'You can close this page. Want to be notified when the reply is ready?'), |
There was a problem hiding this comment.
| notifyBannerText: t('assistant', 'You can close this page. Want to be notified when the reply is ready?'), | |
| notifyBannerText: t('assistant', 'You can close this page. Do you want to be notified when the reply is ready?'), |
| notifyBtnLabel: t('assistant', 'Notify'), | ||
| notifyingBtnLabel: t('assistant', 'Notifying'), |
There was a problem hiding this comment.
| notifyBtnLabel: t('assistant', 'Notify'), | |
| notifyingBtnLabel: t('assistant', 'Notifying'), | |
| notifyBtnLabel: t('assistant', 'Get notified'), | |
| notifyingBtnLabel: t('assistant', 'Notifying'), |
Let's try to stay consistent with what the generic form does. We could add an icon to the button.
- The icon could be
BellOutlineIconwhen the notification is disabled andBellRingOutlineIconwhen it's on. - The button html title could change from
No notificationtoYou will be notified. - The label can be
Get notifiedat all times IMO - Not sure about using the
aria-pressedprop. We could conditionally change the variant or just keep secondary and just be happy with the conditional icon
Wdyt?
| $params = [ | ||
| 'appId' => $task->getAppId(), | ||
| 'id' => $task->getId(), | ||
| 'inputs' => $task->getInput(), |
There was a problem hiding this comment.
We should omit the task input in the notification when it's a chat task. The input can be huge here and we don't use it to render the notification.
This could make the notification system crash if the input is larger than the related column in the notifications table.
- align the notify banner button with the generic task form - omit the task input from chat notifications - wrap the notify banner text instead of truncating it Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
- Test issue, simple fix: do not assert that a notification will have the task input
Otherwise, good to go.
3 participants
Fixes #695
Assisted-by: opencode:glm-5.3