Repository navigation
Conversation
|
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 Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
onKeydownCancelGeneration stops propagation for every Escape key inside the component when assignment and deletion modes are inactive. It does this even when loading.llmGeneration is false. This c… |
Only stop propagation and call cancelGeneration when loading.llmGeneration is true. Let Escape propagate when no generation is active. |
-
Check name Status Explanation Description check The description clearly explains the stop button, Escape-key cancellation, Task Processing cancellation, polling behavior, and host-app handling. It directly matches the changeset. Title check The title clearly and concisely summarizes the main change: allowing users to cancel chat message generation. Linked Issues check The changes meet the coding requirements in #678.InputArea.vueshows a stop button wheneverloading.llmGenerationis true, including empty-input and audio-chat states.ChattyLLMInputForm.vueem…Docstring Coverage No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Full details: Out of Scope Changes check
-
Explanation
onKeydownCancelGenerationstops propagation for every Escape key inside the component when assignment and deletion modes are inactive. It does this even whenloading.llmGenerationis false. This changes host-app Escape behavior outside the generation-cancellation scope of#678.
- Fix all pre-merge checks with AI
- 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
4f5b6e17-46bd-4010-8f73-45da94313915
📒 Files selected for processing (2)
src/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.
Fixes nextcloud#678 Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
9e26cfb to
62dd6c4
Compare
|
All four comments addressed:
Verified manually in a headed browser: stop click during scheduling, cancel of a resumed (reloaded) generation, and Escape not leaking into the host app's own handlers. |
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Nice!
- When submitting a message, the right icon briefly switches to the audio one and then becomes the stop one. Could this be avoided?
- There's a delay of 1 or 2 seconds between the click on "stop" and the UI reaction. Could it be faster? I see the UI reacts after the next
check_generationis done. We could probably consider that if the cancel request is successful, no need to continue polling. We can stop polling and change the form state immediately. Wdyt?
Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
|
Thanks! Both addressed now:
The flicker: the send button now stays visible, disabled, through the whole submit round-trip and the microphone only renders while the input is actually usable. No more detour through the audio icon.
The delay: went with exactly that. When the cancel request succeeds we stop the generation poll and reset the form state right away instead of waiting for the next check_generation tick. The reaction time is now just the cancel round-trip, around 200ms locally where it could hit 2s+ before. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
e11724b5-d799-4860-b1b2-18ed8a9d9c20
📒 Files selected for processing (2)
src/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.
Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
|
The pending stop is now owned by its scheduling request via a sequence number, so a failed scheduling round clears its own stop request and can never trigger or clear another generation's. And the poll handle is captured before the cancel request is sent, so a session switch mid-request can no longer finalize the next generation's poll. |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
c62b0738-517a-4147-b67b-7e909721f090
📒 Files selected for processing: 1
src/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.
Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
|
@julien-nc CodeRabbit reviews currently rate limited, but everything's been addressed so far. |
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Edge case: When cancelling a generation that was triggered by clicking "Regenerate message" on a message, the message that was supposed to be regenerated stays visible in the UI BUT it has actually been deleted on the backend side already.
Switching to another conversation and back to the one (or reloading the page) makes the message disappear (for good reasons, it does not exist anymore).
The same happens for a failed "regeneration" (so there was a bug even before introducing cancellation).
2 ways forward IMO:
- Just make sure the message is removed/deleted in the frontend because we know it is deleted in the backend. Problem: in case of failure/cancellation, the message is gone and one must send another message to continue the conversation.
- Implement a proper fix where the old message is deleted only when the regeneration is successful. This fix does not belong in this PR. It could be a separate issue and a later PR.
I would say: fine with both 1. first and 2. later (let's still create the issue now).
- only finalize the cancelling task's own poll, a session switch during the cancel request must not clobber the next generation's poll handle - drop the regenerated message from the UI when a regeneration is cancelled or fails, the backend already deleted it at scheduling time 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.
🟠 Major · Do not start polling after task ownership is lost. · ChattyLLMInputForm.vue:1204
src/components/ChattyLLM/ChattyLLMInputForm.vue:1204
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not start polling after task ownership is lost.
trackGenerationTaskreturns when the session is inactive, but each caller still starts task-position and generation polling. The session watcher cancels only the poll that exists at the time of the switch. A late scheduling response can install a stale poll and replace the active session’s shared poll handle. The active poll then ignores its responses, so its completion promise can remain pending. The stale task can also reset the active generation state in either finalizer.Check session ownership before starting either poll, and guard generation cleanup with both the session ID and request sequence.
Suggested fix
@@ checkSession(sessionId, isAssignment) if (checkSessionResponseData.messageTaskId !== null) { try { this.loading.llmGeneration = true this.loading.llmRunning = false checkTaskId = checkSessionResponseData.messageTaskId + if (this.active?.id !== sessionId) { + return + } this.trackGenerationTask(checkTaskId, sessionId, this.generationRequestSeq) this.startTaskPositionPolling(checkSessionResponseData.messageTaskId, sessionId)@@ runGenerationTask(sessionId, agencyConfirm = null) console.debug('scheduleGenerationTask response:', generationResponseData) taskId = generationResponseData.taskId this.trackGenerationTask(taskId, sessionId, requestSeq) + if (this.active?.id !== sessionId || requestSeq !== this.generationRequestSeq) { + return + } this.startTaskPositionPolling(taskId, sessionId) const message = await this.pollGenerationTask(taskId, sessionId) @@ } finally { + if (this.active?.id !== sessionId || requestSeq !== this.generationRequestSeq) { + return + } if (this.pollPositionSessionId === sessionId) {Apply the same ownership check before polling and at the start of the
finallyblock inrunRegenerationTask.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a0c5c163-0df9-4f83-b3a1-4cd415d145d8
📒 Files selected for processing: 1
src/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.
|
Thanks for the thorough feedback, @julien-nc.
Went with 1. in f65eceb: a cancelled or failed regeneration now removes the message in the frontend.
|
…stop control 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:
95c4fc96-cef2-47b3-a14c-b24c581595db
📒 Files selected for processing: 2
src/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.
| --> | ||
| <template> | ||
| <div class="container"> | ||
| <div class="container" @keydown="onKeydownCancelGeneration"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Let Escape reach the host when generation is idle.
When focus is inside this container, onKeydownCancelGeneration stops Escape propagation even if loading.llmGeneration is false. cancelGeneration then returns without action. This prevents host-app Escape handlers from working during normal chat use. Check loading.llmGeneration before consuming the key.
Assisted-by: opencode:glm-5.3 Signed-off-by: WSHAPER <42714629+WSHAPER@users.noreply.github.com>
2 participants
While a reply is being generated, the send button turns into a stop button and Escape cancels as well. The underlying Task Processing task is cancelled through the core cancel endpoint, and the generation poll treats a cancelled task as an expected outcome, so the session is usable again right away.
Escape is consumed while a generation is running, so the key press does not leak into the host app. Otherwise, with the assistant opened over Talk, pressing Escape would also deselect the Talk conversation.
Fixes #678
Recon (fully) and implementation (partially) assisted-by: opencode:glm-5.3