Skip to content

Conversation

@edward-ly
Copy link
Contributor

@edward-ly edward-ly commented Apr 27, 2025

Resolves #211. This is a large change that involves reorganizing the locations and properties of multiple components, so there will likely still be some visual and/or functional bugs that need ironing out. Feel free to add such bugs as to-do items here if you find any. Most tasks aside from context chat or context agent have been lightly tested.

Some missing items:

  • move (and possibly reorganize) the loading and scheduled screens (de-scoped in the meantime)
  • fix the New task button not clearing the input prompt

@edward-ly edward-ly requested a review from julien-nc April 28, 2025 18:33
@edward-ly edward-ly marked this pull request as ready for review April 28, 2025 18:33
@edward-ly edward-ly force-pushed the fix/task-layout branch 2 times, most recently from 54191ec to b30edd8 Compare April 29, 2025 07:29
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.

Wow this works really well and looks good already 👍

I didn't review the code yet.

  • It would be nice to select the task sidebar element (set the active prop of the NcAppNavigationItem to true) that is currently displayed (when the item is clicked or when submitting a new task, right after adding the sidebar item)
  • I agree with you, the loading view makes less sense now. It could be replaced by information in the i/o form. For running/scheduled tasks, we still need the "cancel" button to be very visible (not only in the context menu of the sidebar item). It could be next to the submit button. We also need a "get notified" button (like the one in the loading view) if the user wants to get a notification when the task finishes
  • When will we update the data for a running task? The logic could be that if a running/scheduled task is selected, the assistant polls the task info (just like the loading view does)
  • There could also be a single request to taskprocessing/task/{taskId} when selecting a task (any state) to check if it still exists (remove it from the sidebar it if does not) and launch the polling if it is running or scheduled
  • We need to check that the "view result" button in notifications still opens the assistant with the task selected

What do you think?

@edward-ly
Copy link
Contributor Author

edward-ly commented Apr 29, 2025

It would be nice to select the task sidebar element (set the active prop of the NcAppNavigationItem to true) that is currently displayed (when the item is clicked or when submitting a new task, right after adding the sidebar item)

Except that I'm currently reusing the TaskList component which uses NcListItem instead, but that has an active property too, so we can keep using that to preserve the current list formatting. Done.

We need to check that the "view result" button in notifications still opens the assistant with the task selected

Still works for me.

When will we update the data for a running task? The logic could be that if a running/scheduled task is selected, the assistant polls the task info (just like the loading view does)

There could also be a single request to taskprocessing/task/{taskId} when selecting a task (any state) to check if it still exists (remove it from the sidebar it if does not) and launch the polling if it is running or scheduled

Even if a running task is not selected, maybe it would be useful to poll it anyway, so that the entire task list for a selected task type appears to automatically update as tasks are completed.

That and everything else can be added later in a future PR, perhaps.

@julien-nc
Copy link
Member

julien-nc commented Apr 30, 2025

Even if a running task is not selected, maybe it would be useful to poll it anyway, so that the entire task list for a selected task type appears to automatically update as tasks are completed.

I think we should avoid constantly polling to get the status of all tasks.
So we need to choose when the info is refreshed.
IMO:

  • poll a task when it is selected in the UI and its status is scheduled/running
  • when the selected task switches between scheduled/running to success/failure, update all tasks status (just for the selected task type though)
  • when switching task types, update the history for this task type (it is already done, right?)

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.

  • When clicking on "new task" the values are cleared but the sidebar item stays selected. It could just be deselected
  • When switching task types, the info of which task is selected is kept, it could be deselected as well. Now if we select task type TT1, select task TA1, switch to task type TT2, switch back to task type TT1, the TA1 task is still selected in the sidebar but the form is clear.

Code looks good. I agree with all the little fixes you made in AssistantFormInputs.vue and TaskListItem.vue.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

@edward-ly could you add a screenshot or short video so I can do a quick design review? Or is it deployed somewhere? :)

@edward-ly
Copy link
Contributor Author

@edward-ly could you add a screenshot or short video so I can do a quick design review? Or is it deployed somewhere? :)

Sorry about that, thanks for the reminder!

Screenshot 2025-05-05 at 09-37-50 Dashboard - Nextcloud
Screenshot 2025-05-05 at 09-36-47 Dashboard - Nextcloud

@edward-ly
Copy link
Contributor Author

  • When clicking on "new task" the values are cleared but the sidebar item stays selected. It could just be deselected

  • When switching task types, the info of which task is selected is kept, it could be deselected as well. Now if we select task type TT1, select task TA1, switch to task type TT2, switch back to task type TT1, the TA1 task is still selected in the sidebar but the form is clear.

Done for both.

@edward-ly
Copy link
Contributor Author

  • when switching task types, update the history for this task type (it is already done, right?)

Yes.

  • poll a task when it is selected in the UI and its status is scheduled/running

  • when the selected task switches between scheduled/running to success/failure, update all tasks status (just for the selected task type though)

I overall agree, but there are some finer details that need ironing out:

  • What should happen when the user clicks on another task (or even task type) while the current task is being polled (e.g. should the current poll be cancelled)?
  • And when one poll finishes and returns success or failure, should all other scheduled/running tasks be polled too, or is it enough to just call the /api/v1/tasks endpoint? Is polling multiple tasks at the same time even allowed?
  • Maybe it would also be a good idea to disable the form inputs while the currently selected task is scheduled/running, but I don't think there is a way to disable the generic form at the moment.

Maybe these are all questions that should be taken care of in a separate issue/PR.

@julien-nc
Copy link
Member

What should happen when the user clicks on another task (or even task type) while the current task is being polled (e.g. should the current poll be cancelled)?

Yes, and then when switching to a task that is scheduled/running, we should start polling.

And when one poll finishes and returns success or failure, should all other scheduled/running tasks be polled too, or is it enough to just call the /api/v1/tasks endpoint? Is polling multiple tasks at the same time even allowed?

There is no endpoint to poll a list of tasks (that would take a list of task ID as parameter). But I think we don't need it. It is enough for now to only poll the selected task. The flow would be:

  • the user changes the selected task (from task A to B)
  • if task A's state is being polled, stop the polling
  • switch to B, make one single request to get B's state
  • if B is scheduled/running, start polling
  • else just display B

Maybe it would also be a good idea to disable the form inputs while the currently selected task is scheduled/running, but I don't think there is a way to disable the generic form at the moment.

My idea is to replace the form (just the form, not the entire assistant view) with the loading view (the one that contains the NcEmptyContent, the progress etc...) while the task is scheduled/running so there is no need to disable the form. If you prefer getting rid of that loading screen then yes, it seems like a good alternative to disable the form while being in the scheduled/running state (and still display, somewhere above the form: the progress if running, the estimated runtime, a cancel button and a button to toggle the notification for this task).

@julien-nc
Copy link
Member

Maybe these are all questions that should be taken care of in a separate issue/PR.

Yes we can do that in a separate PR.

So the only remaining adjustment to make IMO is to get the task state when selecting a task that we know is scheduled/running. And then if it is still scheduled/running, show the loading view (just like after having submitted a task) so we stay consistent.

@edward-ly edward-ly force-pushed the fix/task-layout branch 2 times, most recently from 7de4372 to eff6e9c Compare May 6, 2025 22:37
@edward-ly
Copy link
Contributor Author

So the only remaining adjustment to make IMO is to get the task state when selecting a task that we know is scheduled/running. And then if it is still scheduled/running, show the loading view (just like after having submitted a task) so we stay consistent.

Still needs some more thorough testing, but I think both of those are working now. Although, I couldn't figure out an elegant way to update the status in the task list as all the pollTask/getTask logic is in assistant.js but getting the updated task list is defined in TaskList.getTasks. I could call getTasks whenever a different task in the list is selected in the meantime, but it doesn't help for the case where the same task is selected (such as the scheduled task while it finishes).

@edward-ly
Copy link
Contributor Author

Latest commit also resolves #217.

@julien-nc julien-nc self-requested a review May 7, 2025 08:42
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.

Thanks for the adjustments. Code looks good.
I tried to check everything. The only thing that is not working is the preservation of the "input" input because the default values are applied in 2 places:

  • AssistantTextProcessingForm::onTaskTypeUserChange (where the input is preserved)
  • AssistantFormInputs::watch::selectedTaskType

and if the second one happens after the first, the input field is cleared.

Let's just remove the last commit and merged this. We can think about #217 in a different PR.

@edward-ly edward-ly merged commit 00f194a into main May 7, 2025
9 checks passed
@edward-ly edward-ly deleted the fix/task-layout branch May 7, 2025 15:01
@jancborchardt jancborchardt moved this to 🎉 Done in 🖍 Design team May 12, 2025
@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

Archived in project

Development

Successfully merging this pull request may close these issues.

Improve task history design

4 participants