Display the task's current position in the queue - #614
Conversation
4e0f58c to
fc2090d
Compare
fc2090d to
8843543
Compare
2e81b98 to
632eb5d
Compare
| reject(new Error('pollTaskPosition request failed')) | ||
| }) | ||
| } | ||
| window.assistantPollPositionTimerId = setInterval(pollPositionOnce, 5000) |
There was a problem hiding this comment.
Should we cancel the previous interval here?
632eb5d to
add1130
Compare
|
Warning Review limit reachedNext included review available in 40 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe assistant and chat interfaces track scheduled task queue positions. Assistant flows start, update, and cancel position polling during task lifecycle changes. Abortable scheduling and stale-response checks prevent outdated task responses from updating state. Assistant and chat components pass task positions to loading content and display localized positions for scheduled tasks. Merge Risk: 🟡 Moderate · up to The change adds recurring queue-position polling for scheduled tasks. At the current head, failures and task transitions can leave polling running, cancel newer tracking, or show stale or missing position information, while one chat path polls more often than the requested five seconds. This creates bounded UI-correctness and request-load risk, so merge should wait for lifecycle and cadence fixes or explicit owner acceptance. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 1 files. (1 skipped: 1 unsupported.) 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 |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds UI support to show a task’s current queue position while it’s scheduled/running, including polling and passing that value through to loading/empty states.
Changes:
- Introduce
taskPositionstate and wire it through Assistant page/modal/form components. - Add queue position display to the running empty content and to the ChattyLLM input placeholder.
- Implement
getTaskPosition()+pollTaskPosition()and add cancellation hooks alongside existing task polling.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/views/AssistantPage.vue | Tracks/resets taskPosition, starts/stops position polling, and passes it to child UI. |
| src/components/RunningEmptyContent.vue | Displays formatted queue position next to runtime during scheduled state. |
| src/components/ChattyLLM/InputArea.vue | Appends queue position to the scheduled placeholder text. |
| src/components/ChattyLLM/ChattyLLMInputForm.vue | Resets loading.taskPosition and fetches position when task is scheduled. |
| src/components/AssistantTextProcessingModal.vue | Adds taskPosition state and passes it to content component. |
| src/components/AssistantTextProcessingForm.vue | Adds taskPosition prop and forwards it to the running area. |
| src/assistant.js | Adds queue-position polling helpers and ensures various flows cancel/reset position polling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export async function cancelTaskPositionPolling() { | ||
| clearInterval(window.assistantPollPositionTimerId) | ||
| window.assistantPollPositionTimerId = null | ||
| } |
| cancelTaskPositionPolling() | ||
| window.assistantPollPositionTimerId = setInterval(pollPositionOnce, 5000) | ||
| // start polling immediately | ||
| pollPositionOnce() |
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
…holder Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
…PollPositionTimerId so pollPositionOnce does not exit and reject the promise Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
… chat UI Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
…inished or failed) Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
add1130 to
1090e93
Compare
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 064a46aa-90fd-445f-99ef-35d07b7d6bb5
📒 Files selected for processing (7)
src/assistant.jssrc/components/AssistantTextProcessingForm.vuesrc/components/AssistantTextProcessingModal.vuesrc/components/ChattyLLM/ChattyLLMInputForm.vuesrc/components/ChattyLLM/InputArea.vuesrc/components/RunningEmptyContent.vuesrc/views/AssistantPage.vue
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| } else if (error.response.data.task_status === TASK_STATUS_INT.scheduled) { | ||
| getTaskPosition(taskId) | ||
| .then(response => { | ||
| const taskPosition = response.data?.ocs?.data | ||
| this.loading.taskPosition = taskPosition | ||
| console.debug('Task position:', taskPosition) | ||
| }) | ||
| .catch(error => { | ||
| console.error('Failed to get task position', error) | ||
| }) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Throttle queue-position requests to the 5-second cadence.
pollGenerationTask() runs every 2 seconds. While the task is scheduled, this branch calls getTaskPosition() on every iteration. Poll queue position separately, or track the last queue-position request time, so this endpoint runs no more than once every 5 seconds.
| }).catch(error => { | ||
| console.debug('[assistant] pollPosition request failed', error) | ||
| clearInterval(window.assistantPollPositionTimerId) | ||
| window.assistantPollPositionTimerId = null | ||
| window.assistantPollPositionPromiseMethods = null | ||
| if (error.status === 404) { | ||
| reject(new Error('task-not-found')) | ||
| return | ||
| } else if (error.status === 412) { | ||
| // the task is not scheduled anymore | ||
| resolve() | ||
| return | ||
| } | ||
| reject(new Error('pollTaskPosition request failed')) | ||
| }) |
| ? this.loading.llmRunning | ||
| ? this.thinkingText | ||
| : this.scheduledText | ||
| + (this.loading.taskPosition ? ' ' + t('assistant', 'Task position: {position}', { position: this.loading.taskPosition }) : '') |
| } else if (error.response.data.task_status === TASK_STATUS_INT.scheduled) { | ||
| getTaskPosition(taskId) | ||
| .then(response => { | ||
| const taskPosition = response.data?.ocs?.data | ||
| this.loading.taskPosition = taskPosition | ||
| console.debug('Task position:', taskPosition) | ||
| }) | ||
| .catch(error => { | ||
| console.error('Failed to get task position', error) | ||
| }) |
1090e93 to
4dea565
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/assistant.js:463
- Every failed position request clears the interval before the status is inspected, so a transient network/5xx failure permanently disables position updates while the task can remain scheduled. Keep retrying transient failures (optionally with backoff), and only stop the interval for cancellation or terminal responses such as 404/412.
if (window.assistantPollPositionTaskId === taskId) {
clearInterval(window.assistantPollPositionTimerId)
window.assistantPollPositionTimerId = null
window.assistantPollPositionTaskId = null
}
src/assistant.js:616
- This POST is no longer tied to an abort controller, but modal close handlers still rely on
cancelTaskPolling()for cleanup. If the modal closes while scheduling is pending, the response handler can run after unmount and start both polling intervals against the closed view. Track the scheduling request separately and either abort it or suppress its continuation after close.
return axios.post(url, params)
| export async function getTask(taskId, signal = null) { | ||
| const { default: axios } = await import('@nextcloud/axios') | ||
| const { generateOcsUrl } = await import('@nextcloud/router') | ||
| const url = generateOcsUrl('taskprocessing/task/{taskId}', { taskId }) | ||
| return axios.get(url, { signal: window.assistantAbortController.signal }) | ||
| const config = signal ? { signal } : {} | ||
| return axios.get(url, config) |
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/assistant.js:470
- This cleanup runs for every failure, including the endpoint's 500 response and transient network errors. Since callers only log the rejection, one temporary failure permanently stops updates and leaves the last queue position displayed while the task remains scheduled. Stop only for terminal/cancellation responses; retry transient failures (with backoff if appropriate), or at minimum clear the displayed position when abandoning polling.
if (window.assistantPollPositionTaskId === taskId) {
clearInterval(window.assistantPollPositionTimerId)
window.assistantPollPositionTimerId = null
window.assistantPollPositionTaskId = null
}
src/assistant.js:511
- This new guard no longer rejects an overlapping response after polling has completed: the terminal-status branch clears
assistantPollTimerIdbut leavesassistantPollTaskIdequal to this task. If two interval requests overlap, a late response can therefore invoke the callback after the promise resolved and overwrite the completed UI with stale task data. Include the cleared-timer condition (or clear the task ID when resolving) to retain the previous stale-response protection.
if (window.assistantPollTaskId !== taskId) {
src/components/ChattyLLM/ChattyLLMInputForm.vue:1079
- The position request is neither awaited nor tied to the selected session/task. A response can arrive after the user switches conversations, after the task starts running, or after generation completes, and then repopulate the shared
loading.taskPositionwith stale data. Track the current generation task (or abort these requests) and verify both session/task identity before assigning the response.
.then(response => {
const taskPosition = response.data?.ocs?.data
this.loading.taskPosition = taskPosition
| preferStreaming: true, | ||
| } | ||
| return axios.post(url, params, { signal: window.assistantAbortController.signal }) | ||
| return axios.post(url, params) |
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
src/assistant.js:478
- The interval is cleared for every failed request, including transient network errors and server 500s, so one temporary failure permanently stops the advertised polling and leaves the last position displayed indefinitely. Stop only for cancellation and terminal 404/412 responses; transient failures should remain retryable (with an appropriate backoff if needed).
This issue also appears on line 840 of the same file.
}).catch(error => {
if (window.assistantPollPositionTaskId === taskId) {
clearInterval(window.assistantPollPositionTimerId)
window.assistantPollPositionTimerId = null
window.assistantPollPositionTaskId = null
}
src/assistant.js:841
- Closing the modal now aborts this request, but the resulting cancellation is handled as a scheduling failure, so an intentional close logs an error and shows the user a failure notification. Detect Axios cancellation before the error handling and return without reporting it.
.catch(error => {
cancelScheduling()
src/assistant.js:921
- This has the same request-ownership race as the other synchronous submission path:
try-againstarts request B immediately after aborting A, then A's catch callscancelScheduling()and aborts B through the shared global controller. Keep the controller local to this invocation, clear the global reference only when it still belongs to this request, and ignore expected cancellation errors.
.catch(error => {
cancelScheduling()
src/assistant.js:519
- This identity check remains true after polling reaches a terminal status because that path clears only the timer, not
assistantPollTaskId. If another interval request is already in flight, it can therefore invoke the callback after finalization and restore stale scheduled/running state. Invalidate the task ID when polling completes so late responses fail this check.
if (window.assistantPollTaskId !== taskId) {
| */ | ||
| export async function scheduleTask(appId, customId, taskType, inputs) { | ||
| window.assistantAbortController = new AbortController() | ||
| export async function scheduleTask(appId, customId, taskType, inputs, signal = null) { |
| .catch(error => { | ||
| cancelScheduling() |
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/components/ChattyLLM/ChattyLLMInputForm.vue:1079
- This asynchronous response is not tied to the session that initiated it. If the user switches sessions while
getTaskPositionis in flight, the old task can overwrite the newly reset position and briefly show the previous session's queue position. Guard the assignment with the capturedsessionId, matching the stale-session checks already used for generation responses.
.then(response => {
const taskPosition = response.data?.ocs?.data
this.loading.taskPosition = taskPosition
| } | ||
| }, | ||
| onCancel() { | ||
| cancelScheduling() |
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
46ccac2 to
2bd7979
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
src/assistant.js:527
- The new stale-response guard uses
assistantPollTaskId, but the terminal-status path clears only the timer. SincesetIntervalcan have overlapping requests, a slower response can arrive after the promise resolves, still pass this guard, and overwrite the completed task state. Clear the active task ID when polling completes so outstanding responses are ignored.
if (window.assistantPollTaskId !== taskId) {
| if (window.assistantPollPositionTaskId === taskId) { | ||
| clearInterval(window.assistantPollPositionTimerId) | ||
| window.assistantPollPositionTimerId = null | ||
| window.assistantPollPositionTaskId = null | ||
| } |
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/assistant.js (1)
481-501: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear
taskPositionwhen queue-position polling stops.The HTTP 412 and request-error branches stop polling without clearing
obj.taskPosition. The callers only log position-poll errors, so the last queue position can remain visible while the task is running or after position polling fails. Clear the target position before resolving or rejecting, or clear it in the corresponding caller handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b2efd97-8e86-490c-ad13-2d1e2d08c640
📒 Files selected for processing (2)
src/assistant.jssrc/views/AssistantPage.vue
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| }).catch(error => { | ||
| console.debug('[assistant] poll request failed', error) | ||
| if (error.status === 404) { | ||
| clearInterval(window.assistantPollTimerId) | ||
| window.assistantPollTimerId = null | ||
| if (window.assistantPollTaskId === taskId) { | ||
| clearInterval(window.assistantPollTimerId) | ||
| window.assistantPollTimerId = null | ||
| window.assistantPollTaskId = null | ||
| } | ||
| reject(new Error('task-not-found')) | ||
| return | ||
| } | ||
| reject(new Error('pollTask request failed')) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop pollTask after a non-404 request failure.
For a 500 or network error, this branch rejects pollTask but does not clear the interval or window.assistantPollTaskId. The interval continues to send requests every two seconds after the caller enters its error handler. The caller resets loading state only for task-not-found, so the UI can remain stuck in the loading state. Clear the owned timer and task ID for every terminal error, then handle the generic error state.
| pollTaskPosition(task.id, this).then(() => { | ||
| console.debug('[assistant] pollTaskPosition finished', task.id) | ||
| }).catch(error => { | ||
| console.debug('[assistant] pollPosition error', task.id, error.message) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target excerpt ---'
sed -n '140,225p' src/views/AssistantPage.vue
printf '%s\n' '--- position/status bindings ---'
rg -n -C 5 'pollTaskPosition|pollTask|getTask|taskPosition|formattedPosition|taskStatus|scheduled' src/views/AssistantPage.vue srcRepository: nextcloud/assistant
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- assistant.js symbols ---'
rg -n -C 12 'export function (pollTaskPosition|pollTask|getTask)|function (pollTaskPosition|pollTask|getTask)|const (pollTaskPosition|pollTask|getTask)|updateTask' src/assistant.js src/views/AssistantPage.vue
printf '%s\n' '--- AssistantPage lifecycle and updateTask ---'
sed -n '80,145p' src/views/AssistantPage.vue
sed -n '245,340p' src/views/AssistantPage.vueRepository: nextcloud/assistant
Length of output: 16978
Set task.status before starting position polling.
The imported pollTaskPosition() updates only taskPosition. The imported pollTask() updates task.status later through this.updateTask. If the position response arrives first, RunningEmptyContent.formattedPosition() hides the position because taskStatus is not TASK_STATUS_STRING.scheduled.
…selected session Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
| if (window.assistantPollTaskId !== taskId) { | ||
| reject(new Error('pollTask cancelled')) |
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/assistant.js:487
- Polling ownership is checked only by
taskId. If polling is restarted for the same task while the previous request is in flight, the old request is aborted, but its rejection sees the same task ID and clears the replacement interval. Track a per-poll controller/token and only stop polling when that token still owns the global state.
if (window.assistantPollPositionTaskId === taskId) {
clearInterval(window.assistantPollPositionTimerId)
window.assistantPollPositionTimerId = null
window.assistantPollPositionTaskId = null
}
| return | ||
| } | ||
| reject(new Error('pollTask request failed')) | ||
| console.warn('[assistant] poll temporary failure, will retry', error) |
Poll the selected task's position every 5 seconds. Display it in the "loading" empty content.
Todo
🤖 AI (if applicable)