Duplicate tags and assignees #1905

New Issue

saibotk · 2022-02-16T15:19:24Z

saibotk commented

2022-02-16 15:19:24 +00:00

Sometimes when using the kanban view, we experience duplicate tags and assignees, as shown below:

To reproduce:

Edit the description or priority of a tag
Add a new tag or assignee
See the entry being duplicated

Sometimes when using the kanban view, we experience duplicate tags and assignees, as shown below: ![image](/attachments/dfd1d398-4cad-4c52-abae-16e3607cddc8) To reproduce: 1. Edit the description or priority of a tag 2. Add a new tag or assignee 3. See the entry being duplicated

image.png

30 KiB

dpschen commented

2022-02-16 16:05:33 +00:00

Thanks for the issue, sadly I could'nt reproduce on https://try.vikunja.io

Can you tell the exact steps that you did to get there?

Thanks for the issue, sadly I could'nt reproduce on https://try.vikunja.io Can you tell the exact steps that you did to get there?

konrad commented

2022-02-16 19:42:39 +00:00

A fix for this might be to filter everything for duplicate IDs when showing it but I feel like this would only fix the symptoms of the bug, not the bug itself.

konrad added the

kind/bug

label 2022-02-16 19:42:47 +00:00

dpschen commented

2022-02-17 06:51:38 +00:00

Let’s not start with a workaround like this, where we don’t know the origin of the error at all. If we add a fix like that we might add other unintended bugs along the way. Better to watch out for the source and understand, why it happens.

👍 1

saibotk commented

2022-02-17 12:15:39 +00:00

Yes i will just take a look at the code of the latest release and test it.
What would be super cool, is if we could then backport a fix and release it, since this would be really nice to have as quickly as possible because it makes vikunja seem very buggy and is quite annoying. Is there any chance we could do that, once i provide a patch?

Yes i will just take a look at the code of the latest release and test it. What would be super cool, is if we could then backport a fix and release it, since this would be really nice to have as quickly as possible because it makes vikunja seem very buggy and is quite annoying. Is there any chance we could do that, once i provide a patch?

saibotk commented

2022-02-18 01:22:20 +00:00

Well i digged a little deeper into the issue and it seems that it is due to some state sync issues between the vuex task store and the local task shown to be edited.

Once the the task is saved by editing any other value like the description, it is then set to the instance that was saved to the vuex store and thus reacts to changes.
This differs from the initial state, where the task is loaded from the backend and thus does not care about the vuex addLabel method being called and the local state being modified.

So the issue is actually two-fold:

The initial loading of a task is never synced to the vuex store (which also is only really used in kanban board popup style), thus defeating the single source of truth principle.
The multiselect component is not side-effect free and will add new elements to its internal array, which can also be the actual label array on the task object, and still emits an event with the new list.

Still need to look into why this is currently fixed. Stay tuned.

But if this did not change that much, id suggest discussing a strategy to refactor those components. Currently for all editing components we are always emitting the events to signal an update and also at the same time modify them via the v-model sync.
This could all be avoided, if we would use a single approach and let the outermost component handle updating the state on signals from lower components like the labelEdit. The lower components then might follow the principle of only signalling the intention and displaying the modeled data instead of additionally manipulating the data themselves.

What do you think @dpeschen @konrad ?

Well i digged a little deeper into the issue and it seems that it is due to some state sync issues between the vuex task store and the local task shown to be edited. Once the the task is saved by editing any other value like the description, it is then set to the instance that was saved to the vuex store and thus reacts to changes. This differs from the initial state, where the task is loaded from the backend and thus does not care about the vuex addLabel method being called and the local state being modified. So the issue is actually two-fold: 1. The initial loading of a task is never synced to the vuex store (which also is only really used in kanban board popup style), thus defeating the single source of truth principle. 2. The multiselect component is not side-effect free and will add new elements to its internal array, which can also be the actual label array on the task object, and still emits an event with the new list. Still need to look into why this is currently fixed. Stay tuned. But if this did not change that much, id suggest discussing a strategy to refactor those components. Currently for all editing components we are always emitting the events to signal an update and also at the same time modify them via the v-model sync. This could all be avoided, if we would use a single approach and let the outermost component handle updating the state on signals from lower components like the labelEdit. The lower components then might follow the principle of only signalling the intention and displaying the modeled data instead of additionally manipulating the data themselves. What do you think @dpeschen @konrad ?

saibotk commented

2022-02-18 01:43:27 +00:00

On the master branch this might have been fixed by:

ff9e1b3fca

Since this will "clone" the task or rather only add the label to the stored task and remove the side-effect.

On the master branch this might have been fixed by: https://kolaente.dev/vikunja/frontend/commit/ff9e1b3fcad02fad290c1b663d33e640f85a8a8c Since this will "clone" the task or rather only add the label to the stored task and remove the side-effect.

dpschen commented

2022-02-18 11:12:45 +00:00

Hey @saibotk,

thanks for looking into this! First of there is also vikunja/frontend#1534 coming up that should resolve similar bugs to the commit you linked to ff9e1b3fca.

For myself I can say that I was aware of a lot of the issues that you describe. This is also one of the reasons why a new release takes so long. We have to convert to vue 3, script setup and, in order to help us find bugs, convert to Typescript.

I was not aware of the details for this specific case though!!

id suggest discussing a strategy to refactor those components.

I have already started doing this in a branch in my local repository, but it broke too much so it was not commitable. Now these changes might be outdated already (have to check).

Not 100% the topic of this issue but also in the area of the kanban view:
I started a branch where I separate the bucket as an extra component to reduce complexity. I'm telling this in order to reduce possible merge conflicts. While doing this I also converted the ListKanban.vue and the new Bucket.vue to script setup and Typescript.

The initial loading of a task is never synced to the vuex store (which also is only really used in kanban board popup style), thus defeating the single source of truth principle.

This is a problem we have currently everywhere in the codebase.
The bad thing about this current situation is that without rewriting a lot it's basically not possible to deal with this without a lot of hacks and workarounds.
By creating the useTaskList function (in /src/composables/taskList.js) I started centralizing the task management / storage.
I also played around with Pinia in vikunja/frontend#1098 (maybe a future store candidate) but must say that I'm really not happy with the new API compared to vuex. I would have loved more api improvements.

The multiselect component is not side-effect free and will add new elements to its internal array, which can also be the actual label array on the task object, and still emits an event with the new list.

This is an issue that I didn't look into yet. Do you want to check that out?
In my view the multiselect component is to complex. But for solving the issue a quick fix might be more appropriate

This could all be avoided, if we would use a single approach and let the outermost component handle updating the state on signals from lower components like the labelEdit. The lower components then might follow the principle of only signalling the intention and displaying the modeled data instead of additionally manipulating the data themselves.

I think I didn't touch labelEdit, so feel free to dig in there =)

Hopefully I didn't confuse you too much :D

Hey @saibotk, thanks for looking into this! First of there is also https://kolaente.dev/vikunja/frontend/pulls/1534 coming up that should resolve similar bugs to the commit you linked to ff9e1b3fca. For myself I can say that I was aware of a lot of the issues that you describe. This is also one of the reasons why a new release takes so long. We have to convert to vue 3, script setup and, in order to help us find bugs, convert to Typescript. I was not aware of the details for this specific case though!! > id suggest discussing a strategy to refactor those components. I have already started doing this in a branch in my local repository, but it broke too much so it was not commitable. Now these changes might be outdated already (have to check). Not 100% the topic of this issue but also in the area of the kanban view: I started a branch where I separate the bucket as an extra component to reduce complexity. I'm telling this in order to reduce possible merge conflicts. While doing this I also converted the `ListKanban.vue` and the new `Bucket.vue` to script setup and Typescript. > The initial loading of a task is never synced to the vuex store (which also is only really used in kanban board popup style), thus defeating the single source of truth principle. This is a problem we have currently everywhere in the codebase. The bad thing about this current situation is that without rewriting a lot it's basically not possible to deal with this without a lot of hacks and workarounds. By creating the useTaskList function (in `/src/composables/taskList.js`) I started centralizing the task management / storage. I also played around with Pinia in https://kolaente.dev/vikunja/frontend/pulls/1098 (maybe a future store candidate) but must say that I'm really not happy with the new API compared to vuex. I would have loved more api improvements. > The multiselect component is not side-effect free and will add new elements to its internal array, which can also be the actual label array on the task object, and still emits an event with the new list. This is an issue that I didn't look into yet. Do you want to check that out? In my view the multiselect component is to complex. But for solving the issue a quick fix might be more appropriate > This could all be avoided, if we would use a single approach and let the outermost component handle updating the state on signals from lower components like the labelEdit. The lower components then might follow the principle of only signalling the intention and displaying the modeled data instead of additionally manipulating the data themselves. I think I didn't touch labelEdit, so feel free to dig in there =) Hopefully I didn't confuse you too much :D

saibotk commented

2022-02-18 11:44:22 +00:00

No, that is fine, im quite familiar with Vue myself, but never really had to use vuex much. :)

And yes while digging around i was just remembered what a pain it is to develop with Vue 2 and plain JS instead of a fully typed setup with Vue 3 :D

Thanks for the insight, I think I will try to get a fix for the main issue up that is also applicable to be backported and then i might even be up to refactor the component more for the next version.

No, that is fine, im quite familiar with Vue myself, but never really had to use vuex much. :) And yes while digging around i was just remembered what a pain it is to develop with Vue 2 and plain JS instead of a fully typed setup with Vue 3 :D Thanks for the insight, I think I will try to get a fix for the main issue up that is also applicable to be backported and then i might even be up to refactor the component more for the next version.

dpschen commented

2022-02-18 11:50:08 +00:00

And yes while digging around i was just remembered what a pain it is to develop with Vue 2 and plain JS instead of a fully typed setup with Vue 3 :D

Indeed! 😆 The codebase is so large that it sadly takes a lot of time to convert.
I'm very familiar with vuex, but not too much with Typescript… As far as I know the both aren't perfectly well suited for each other. That doesn't help…

Thanks for the insight, I think I will try to get a fix for the main issue up that is also applicable to be backported and then i might even be up to refactor the component more for the next version.

That would be amazing, thanks =)

> And yes while digging around i was just remembered what a pain it is to develop with Vue 2 and plain JS instead of a fully typed setup with Vue 3 :D Indeed! 😆 The codebase is so large that it sadly takes a lot of time to convert. I'm very familiar with vuex, but not too much with Typescript… As far as I know the both aren't perfectly well suited for each other. That doesn't help… > Thanks for the insight, I think I will try to get a fix for the main issue up that is also applicable to be backported and then i might even be up to refactor the component more for the next version. That would be amazing, thanks =)

👍 1

konrad commented

2022-02-19 21:30:47 +00:00

Wow thanks for that in-depth analysis!

A backport would certainly be an option but since we migrated to vue 3 since the last release I fear it would mean we will need to write two versions of the patch - I hope to get another release out in march or april so maybe backporting wouldn't be worth the hassle.

This differs from the initial state, where the task is loaded from the backend and thus does not care about the vuex addLabel method being called and the local state being modified.

IIRC my idea with that was to avoid reloading the task after an edit was made. Theoretically it should be enough to just do a task.labels.push(newLabel) to the task in store. Guess that didn't work out so well.

Not quite sure how to solve it? Maybe every time we load a single task from the api we should also put it in store? Would that actually solve the problem with labels?

I think in the longer run we should put all tasks in store and modify them there to enable things like global popups and offline storage. But that will be quite a bit of work.

Open for suggestions here.

The multiselect component is a topic for itself 🙃 Back in the day, I used a vue package for that which had a lot of switches and options to customize it. I didn't use half of them so I though I would be better off just building my own. Now I know why that package had so many bells and whistles... I'm pretty sure the one I built for Vikunja could be optimized further, maybe refactored into multiple components instead of one giant one with props for everything.

Thanks for the insight, I think I will try to get a fix for the main issue up that is also applicable to be backported and then i might even be up to refactor the component more for the next version.

Awesome, a fix would be great!

Wow thanks for that in-depth analysis! A backport would certainly be an option but since we migrated to vue 3 since the last release I fear it would mean we will need to write two versions of the patch - I hope to get another release out in march or april so maybe backporting wouldn't be worth the hassle. > This differs from the initial state, where the task is loaded from the backend and thus does not care about the vuex addLabel method being called and the local state being modified. IIRC my idea with that was to avoid reloading the task after an edit was made. Theoretically it should be enough to just do a `task.labels.push(newLabel)` to the task in store. Guess that didn't work out so well. Not quite sure how to solve it? Maybe every time we load a single task from the api we should also put it in store? Would that actually solve the problem with labels? I think in the longer run we should put all tasks in store and modify them there to enable things like global popups and offline storage. But that will be quite a bit of work. Open for suggestions here. The multiselect component is a topic for itself 🙃 Back in the day, I used a vue package for that which had a lot of switches and options to customize it. I didn't use half of them so I though I would be better off just building my own. Now I know why that package had so many bells and whistles... I'm pretty sure the one I built for Vikunja could be optimized further, maybe refactored into multiple components instead of one giant one with props for everything. > Thanks for the insight, I think I will try to get a fix for the main issue up that is also applicable to be backported and then i might even be up to refactor the component more for the next version. Awesome, a fix would be great!

konrad commented

2022-02-19 21:57:12 +00:00

Another case I just found that's probably related to this: Removing labels from a task doesn't seem to work at all - the labels are still on that task. There's not even a request to the api happening here so maybe not related.

Reproduced in 0.18.1+1161-e14cfd985c

Another case I just found that's probably related to this: Removing labels from a task doesn't seem to work at all - the labels are still on that task. There's not even a request to the api happening here so maybe not related. Reproduced in 0.18.1+1161-e14cfd985c

dpschen commented

2022-02-20 11:25:53 +00:00

Another case I just found that's probably related to this: Removing labels from a task doesn't seem to work at all - the labels are still on that task. There's not even a request to the api happening here so maybe not related.

Reproduced in 0.18.1+1161-e14cfd985c

Should we create a new ticket?

> Another case I just found that's probably related to this: Removing labels from a task doesn't seem to work at all - the labels are still on that task. There's not even a request to the api happening here so maybe not related. > > Reproduced in 0.18.1+1161-e14cfd985c Should we create a new ticket?

konrad commented

2022-02-20 16:46:23 +00:00

I think so yeah. Already added an item in the backlog though.

👍 1

konrad commented

2023-03-09 11:57:46 +00:00

Is this still relevant?

saibotk commented

2023-03-09 12:43:35 +00:00

No as it was fixed with the refactor.

I think we were just unsure if we would want to actually try looking into this to a broader extent because it seemed to me that it was only fixed due to a coincidental change.

But id suggest closing this issue regardless as rhe code may change when the architecture regarding the store changes. Additionally im not in the loop about all the more recent changes since then and would consider this outdated.

Thanks!

No as it was fixed with the refactor. I think we were just unsure if we would want to actually try looking into this to a broader extent because it seemed to me that it was only fixed due to a coincidental change. But id suggest closing this issue regardless as rhe code may change when the architecture regarding the store changes. Additionally im not in the loop about all the more recent changes since then and would consider this outdated. Thanks!

konrad closed this issue

2023-03-09 12:45:17 +00:00

konrad referenced this issue from a commit

2024-02-07 14:00:34 +00:00

fix(deps): update dependency @vueuse/core to v8.4.2 (#1905)

Sign in to join this conversation.