[Bug/v2]: ComboboxItem unhandled error in mounted

[RayGuo-ergou]

Environment

Developement/Production OS: Windows 11
Node version: 20
Package manager: [email protected]
Reka Vue version: 0.0.2
Vue version: ^3.0.0
Client OS: Windows 11
Browser: Firefox

Link to minimal reproduction

https://github.com/RayGuo-ergou/reka-reproduction

Steps to reproduce

Clone the repo, run the dev server, then in app search anything, delete all text then search again that will make the computed as empty array.

const people = [
  { id: 1, name: 'Durward Reynolds'},
  { id: 2, name: 'Kenton Towne'},
  { id: 3, name: 'Therese Wunsch'},
  { id: 4, name: 'Benedict Kessler'},
  { id: 5, name: 'Katelyn Rohan'},
]
const selectedPeople = ref(people[0])
const search = ref('')

const searchResult = computed(()=>{
  return people.filter(i => !i.name.startsWith(search.value))
})

Warning

[Vue warn]: Unhandled error during execution of mounted hook
at <ComboboxItem key=1 value=
Object { id: 1, name: "Durward Reynolds" }

....

Error

Uncaught (in promise) TypeError: can't access property "textContent", u.value is undefined

Describe the bug

I was trying v2 aka reka with shadcn-ui, and I found that a breaking changing in combobox may break the tags-input

In ee8a3f2#diff-19766b3de8934964a0c2730b5e4d28c90759d1b67572135ba3be029752b4d31dR50

The currentElement will be undefined if the list of items become an empty array.

Expected behavior

No response

Context & Screenshots (if applicable)

[zernonia]

@RayGuo-ergou v2 has breaking changes, and migration for shadcn-vue will not be so direct. So can I close this ticket off as it's not a bug but something needs to be updated.

[RayGuo-ergou]

So can I close this ticket off as it's not a bug but something needs to be updated.

Yeah sure thing, but just want to ask "need to be updated" do you mean reka side or shadcn side?

Or do you mean v2 is still working in progress so this will be updated later?🤔

This issue is not particular for shadcn in my opinion, but combo box item does not support reactive array with v-for.

[RayGuo-ergou]

Never mind ^, both indeed have this issue, and my reproduction is based on https://github.com/radix-vue/radix-vue/blob/ee8a3f2366a5c27c2bf1cc0a1ecbb0fea559a9f7/packages/radix-vue/src/Combobox/story/_ComboboxManualFilter.vue

except I am using native javascript startsWith

So I would say this is a bug.

[RayGuo-ergou]

Hi @zernonia Sorry for pinging, but I guess you only have notifications for pinged comment?

I have came to a conclusion that this is a bug with combobox in v2, for example I can even trigger this using example from the doc.

Here's an screenshot from the story in v2.

Sorry for not describe this issue clearly from beginning, and I understand v2 in very early stage so my intention is mostly let you know this issue.

I am also happy to look into this sometime next month :-).

[zernonia]

@RayGuo-ergou can you check again with reka alpha release

[RayGuo-ergou]

Hi @zernonia thanks for asking, no with alpha release it still have the same issue. Same for the example from the store, and it's very weird that initially works fine then after few random actions (e.g. input, remove input and refresh the page) it then always throws the error above.

Apologies to make a promise but did not do it last month. I took some days just resting, will try to look into it this weekend.

[zernonia]

Can you please share the reproduction using this stackblitz? https://stackblitz.com/edit/hn2yox?file=src%2FApp.vue

[RayGuo-ergou]

The items has to be fetched from a server to reproduce this.
Here's the link: https://stackblitz.com/edit/hn2yox-yy4md2?file=src%2FApp.vue

For me after the data fetched I will see this.

[RayGuo-ergou]

Hi @zernonia

The patch that seems to fix this:

diff --git a/packages/core/src/Combobox/ComboboxItem.vue b/packages/core/src/Combobox/ComboboxItem.vue
index a03aec8b..a59f5e4a 100644
--- a/packages/core/src/Combobox/ComboboxItem.vue
+++ b/packages/core/src/Combobox/ComboboxItem.vue
@@ -44,10 +44,16 @@ if (!props.value) {
 }
 
 const isRender = computed(() => {
-  if (rootContext.isVirtual.value || rootContext.ignoreFilter.value || !rootContext.filterState.search)
+  if (rootContext.isVirtual.value || rootContext.ignoreFilter.value || !rootContext.filterState.search) {
     return true
-  else
-    return rootContext.filterState.filtered.items.get(id)! > 0
+  }
+  else {
+    const filteredCurrentItem = rootContext.filterState.filtered.items.get(id)
+    if (filteredCurrentItem === undefined) {
+      return true
+    }
+    return filteredCurrentItem > 0
+  }
 })
 
 onMounted(() => {

---

Because in case of async fetching data to use for comboboxItems, isRender is false for the new items(will explain below). And then after getting the data from backend, as onMounted call already finished. So rootContext.allItems.value.set will not execute when isRender become true. These new "items" are lost.

Why the error appear

The happens before what happened above.

  if (rootContext.isVirtual.value || rootContext.ignoreFilter.value || !rootContext.filterState.search) {

^ This is false in this case. So it then goes to else block and returns false for isRender because undefined > 0 === false

Thus, ListboxItem is not rendered, which makes currentElement is undefined. So it triggers the error shows above.

I am not sure if this is the approach/correct-fix or you prefer other ways. so I leave as a comment rather than PR. Let me know if you are happy for this to be a PR :). Have created a PR think that would be clear to show the changes.

[RayGuo-ergou]

Having a second thought, this bug is not limited to async render. It applies to all items if if (rootContext.isVirtual.value || rootContext.ignoreFilter.value || !rootContext.filterState.search) { is false.

And there's an issue in my code is the first search will not apply filter(if the list is never opened). Will look into that later.

[sunnylost]

This bug made me realize that the currentElement obtained from usePrimitiveElement() could be undefined, so optional chaining should be used instead of directly accessing the property.

[RayGuo-ergou]

This bug made me realize that the currentElement obtained from usePrimitiveElement() could be undefined, so optional chaining should be used instead of directly accessing the property.

Right, at the moment currentElement is declared as ref<HtmlElement>. Perhaps should change to ref<undefined|HtmlElement> fisrt?