Repository navigation
feat(chat): offer a notification when a reply is ready - #697
Conversation
Fixes nextcloud#695 Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 8 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly describes the main change: adding notifications when a chat reply is ready. Description check The description is directly related to the changes. It identifies issue #695and explains the browser notification dependency on the background push click fix.Linked Issues check The changes satisfy the coding requirements in #695.InputAreashows a delayed banner during generation, explains that the page can close, and provides a per-reply notify control. `ChattyLLMInputFor…Out of Scope Changes check The changes remain within #695. The frontend changes implement the banner, delay, dismissal, permission flow, and task-state synchronization. The backend changes implement chat success and failure not…Full details: Linked Issues check
-
Explanation
The changes satisfy the coding requirements in
#695.InputAreashows a delayed banner during generation, explains that the page can close, and provides a per-reply notify control.ChattyLLMInputFormrequests permission and synchronizes the notification state with the active task. Backend listeners send success and failure notifications after reply persistence.Notifiersupplies conversation links and aView replyaction. URL handling opens the relevant chat. Added unit tests cover notification creation, links, subjects, and chat task detection. Closed-tab click delivery still depends on the external background-push fix named in#695.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within
#695. The frontend changes implement the banner, delay, dismissal, permission flow, and task-state synchronization. The backend changes implement chat success and failure notifications and preserve generic task handling. URL routing and unit tests support the requested notification behavior. No unrelated product change or ignored file appears in the reviewed scope.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 8 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- 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.
The notification service omits the input for chat tasks since it is never rendered and can overflow the notifications table column. 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 · Preserve assignment context in generic chat notification links. · Notifier.php:417-431
lib/Notification/Notifier.php:417-431
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve assignment context in generic chat notification links.
When
notifyWhenReadyis called for an assigned ChattyLLM task, the generic notification path can create achat_successorchat_failurenotification. Its fallback URL contains onlysessionId. The assistant then opens chat mode, where assigned sessions are excluded, so the notification does not select the relevant assignment conversation.Propagate an assignment flag with the generic notification and add
isAssignment=trueonly for assigned sessions. Keep ordinary chat links unchanged.
🟡 Minor · Require notifications support for closed-tab clicks. · Notifier.php:417-431
lib/Notification/Notifier.php:417-431
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire notifications support for closed-tab clicks.
Notifications v35.0.2 has no background
notificationclickhandler. The chat notification supplies aWEBaction, but the assistant only provides a fallback fortasknotifications. When the tab is closed, clicking the chat notification can therefore fail to open the conversation.Declare a minimum notifications release that contains backport
#3480, or add an equivalent fallback.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
97f74648-fda5-459d-beac-f4c6972cf655
📒 Files selected for processing: 3
lib/Service/NotificationService.phpsrc/components/ChattyLLM/InputArea.vuetests/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.
…en-ready Resolve the ChattyLLM conflicts against the cancel-generation work that landed upstream (nextcloud#696 + follow-ups): - unify on upstream's generationTaskId tracking (request sequences, pending-cancel handshake, ownership-guarded resets) and layer the notify-when-ready state on top: trackGenerationTask() resets the toggle, the notify state sync and onBackgroundNotify read generationTaskId instead of the now removed currentGenerationTaskId - InputArea: keep both the notify banner and the stop-generation control, and merge the two loading.llmGeneration watchers into one Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
The browser permission prompt can stay open long enough for the tracked generation to end or be replaced: applying the toggle then would target the wrong task and desync the banner state. Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
I don't think this path is reachable with the current UI:
To hit the wrong link you'd have to create a notify-ready row for an assignment task id via the API directly. If that case is worth covering I can pass an assignment flag along, but I'd leave it out otherwise. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments: 1
src/components/ChattyLLM/ChattyLLMInputForm.vue: 1
1412-1412: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck task ownership before applying the update.
If the user switches conversations or starts another generation while
setNotifyReadyis pending, this callback setsisNotifyEnabledfor the old task on the new generation. A fresh permission grant can also schedule an unrelated page reload. Check thatgenerationTaskId === taskIdbefore updating the toggle or scheduling the reload.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
89ae1b04-2bc9-4a43-93a3-fff346e2831c
📒 Files selected for processing: 6
lib/Listener/TaskFailedListener.phplib/Listener/TaskSuccessfulListener.phplib/Notification/Notifier.phplib/Service/NotificationService.phpsrc/components/ChattyLLM/ChattyLLMInputForm.vuesrc/components/ChattyLLM/InputArea.vue
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>
The notify listeners were registered before the chat persistence listener, so an opted-in chat task could push the ready notification before the assistant message is inserted and the user would open a conversation whose reply does not exist yet. Register ChattyLLMTaskListener first so the reply is saved before any notification goes out. Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
The reload timer after a fresh browser notification grant only checked isNotifyEnabled: a toggle back to disabled whose request was still pending kept it true, so the reload could interrupt the opt-out and the notification stayed enabled server side. Capture the per-request sync id and let only the newest toggle's response and reload timer apply. Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c2749bc2-915e-4dc2-99c2-cbd77a097814
📒 Files selected for processing: 2
lib/AppInfo/Application.phpsrc/components/ChattyLLM/ChattyLLMInputForm.vue
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
Two overlapping toggle requests could complete out of order server side: a late enable POST landing after an opt-out DELETE would restore the notification while the client keeps showing it disabled. Ignore further toggles while one is in flight, also covering double clicks during the browser permission prompt. Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
|
FYI ping: @julien-nc everything should be addressed so far. |
3 participants
Fixes #695
Assisted-by: opencode:glm-5.3