Skip to content

Conversation

edward-ly
Copy link
Contributor

@edward-ly edward-ly commented Jun 11, 2025

Fixes #245. In addition, the task list is automatically updated when a new task is submitted or a running task is canceled, with the submitted task also being automatically selected from the list.

@edward-ly edward-ly requested a review from julien-nc June 11, 2025 22:50
@edward-ly edward-ly marked this pull request as ready for review June 11, 2025 22:50
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! A few things to adjust and we're good to go.

The task list item is not updated when the task finishes. Maybe we should fire an event in the event bus after pollTask so the TaskList component can update its internal task object status.
emit('task-updated', task)
which would be caught in TaskList and call a method like:

updateTask(task) {
    const internalTask = this.tasks.find(t => t.id === task.id)
    if (internalTask) {
        internalTask.status = task.status
    }
}

@edward-ly
Copy link
Contributor Author

edward-ly commented Jun 12, 2025

The task list item is not updated when the task finishes. Maybe we should fire an event in the event bus after pollTask so the TaskList component can update its internal task object status. emit('task-updated', task) which would be caught in TaskList and call a method like:

updateTask(task) {
    const internalTask = this.tasks.find(t => t.id === task.id)
    if (internalTask) {
        internalTask.status = task.status
    }
}

Is emit('task-updated', task) being called in assistant.js when the task finishes or somewhere else? How can the TaskList component listen to an emit from a grandparent?

Another alternative could be instead of declaring the tasks list in the TaskList component data, declare it in assistant.js instead and pass it down to TaskList as a component property.

@julien-nc
Copy link
Member

Another alternative could be instead of declaring the tasks list in the TaskList component data, declare it in assistant.js instead and pass it down to TaskList as a component property.

It's a lot of chaining props to carry the value from assistant.js to TaskList. I would be in favor of using the event bus.

When using this function: import { emit } from '@nextcloud/event-bus' you can emit an event+data in the event bus.
You can subscribe to this event anywhere, this is independent from Vue.
In TaskList you can use import { subscribe } from '@nextcloud/event-bus' to subscribe to the event. You can do so in the mounted function:
subscribe('task-updated', this.updateTask)
and you need to unsubscribe when the TaskList component is destroyed, in the beforeDestroy block:
unsubscribe('task-updated', this.updateTask)

@edward-ly edward-ly force-pushed the fix/loading-view branch 2 times, most recently from bcdb3d1 to 08c1af1 Compare June 13, 2025 17:02
@edward-ly
Copy link
Contributor Author

edward-ly commented Jun 13, 2025

When using this function: import { emit } from '@nextcloud/event-bus' you can emit an event+data in the event bus. You can subscribe to this event anywhere, this is independent from Vue. In TaskList you can use import { subscribe } from '@nextcloud/event-bus' to subscribe to the event. You can do so in the mounted function: subscribe('task-updated', this.updateTask) and you need to unsubscribe when the TaskList component is destroyed, in the beforeDestroy block: unsubscribe('task-updated', this.updateTask)

done in 08c1af1, but using Object.assign(taskToUpdate, task) instead of taskToUpdate.status = task.status so that all task properties are updated

@edward-ly edward-ly requested a review from julien-nc June 13, 2025 17:03
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great. @janepie Could you give it a try as well?
@edward-ly Let's wait for Jana's feedback to merge this if you don't mind.

@julien-nc julien-nc requested a review from janepie June 16, 2025 15:34
@janepie
Copy link
Member

janepie commented Jun 17, 2025

Nice one!
One thing: when I submit a task and then switch task type, I stay in the loading screen and cannot leave it in any way, even the buttons do not work anymore

@julien-nc
Copy link
Member

@edward-ly You can rebase your branch on main where part of the task type switch issue was fixed (by #262).
The "task loading" state is still not reset correctly when switching task types.

@edward-ly
Copy link
Contributor Author

edward-ly commented Jun 17, 2025

Nice one! One thing: when I submit a task and then switch task type, I stay in the loading screen and cannot leave it in any way, even the buttons do not work anymore

Good catch, should be fixed in 12ecdad now, mind testing one last time? @janepie @julien-nc

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with switching task types while a task is "loading" is fixed when opening the assistant with the menu entry but not when opening it from a notification. See inline comment.

@janepie
Copy link
Member

janepie commented Jun 17, 2025

One more small thing: If I switch tasks or task types, I get a loading spinner on the Submit button and it doesn't disappear even if there is no task running
image
Do we still need this anywhere or is this a left-over from before?

@edward-ly
Copy link
Contributor Author

edward-ly commented Jun 17, 2025

One more small thing: If I switch tasks or task types, I get a loading spinner on the Submit button and it doesn't disappear even if there is no task running. Do we still need this anywhere or is this a left-over from before?

Not necessarily the red color, but I do see the button being in the wrong state too, latest commit should fix it.

@janepie feel free to merge this if everything looks good to you

…tion property

Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
… click, toggle notification request instead

Signed-off-by: Edward Ly <contact@edward.ly>
…ng screen depending on task status

Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
@janepie
Copy link
Member

janepie commented Jun 18, 2025

I still have the loading spinner, I just made a recording

chrome-capture-2025-6-18

Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
Signed-off-by: Edward Ly <contact@edward.ly>
@edward-ly
Copy link
Contributor Author

I still have the loading spinner, I just made a recording

Ah I see, I fixed the case for when a task from the task list is loaded, but not when switching task types or creating a new task, should be fixed now.

Copy link
Member

@janepie janepie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, really nice work!

@janepie janepie merged commit 87655da into main Jun 20, 2025
16 checks passed
@janepie janepie deleted the fix/loading-view branch June 20, 2025 10:02
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jun 21, 2025
* Adjust `get notified` string.
* Check for changed icon rather than message.

Changes caused by
nextcloud/assistant#256

Signed-off-by: Max <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jun 21, 2025
* Adjust `get notified` string.
* Check for changed icon rather than message.

Changes caused by
nextcloud/assistant#256

Signed-off-by: Max <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jun 21, 2025
* Adjust `get notified` string.
* Check for changed icon rather than message.

Changes caused by
nextcloud/assistant#256

Signed-off-by: Max <max@nextcloud.com>
@julien-nc julien-nc mentioned this pull request Jun 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't block the screen with the loading view
3 participants