fix(Tree): prefer value for default item keys

[onmax]

Without a custom getKey prop, getItemKey() defaults to the item's label. Items with the same label at different nesting levels (e.g. two folders named "references") share expansion/selection state.

The fix I implemented is a to build a WeakMap mapping each item to a unique index-based path (e.g. 0, 1-0, 1-0-0). Uses toRaw() to handle Vue reactive proxies. Backward-compatible: users with getKey prop are unaffected.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request modifies the key resolution logic in the Tree component to change the priority for determining item expansion keys. A new helper function getItemValueKey was introduced to extract string representations from an item's value property, and the getItemKey function was updated to prefer value-derived keys over label-derived keys. A corresponding test case was added to verify that items are now keyed by their value before falling back to their label for default expansion behavior.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check	✅ Passed	The PR description explains the problem (items with same label share state) and solution (prefer value over label as default key), which directly relates to the code changes.
Title check	✅ Passed	The title 'fix(Tree): prefer value for default item keys' clearly and concisely summarizes the main change—prioritizing item.value over label for default tree keys.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

test/components/Tree.spec.ts (1)

117-125: Variable shadowing: items shadows outer scope.

The local items (line 117) and updatedItems (line 123) shadow the test file's items fixture at line 13. While scoping is correct, renaming improves clarity:

♻️ Suggested rename

-    const items = wrapper.findAll('[role="treeitem"]')
-    const nestedReferences = items.find(item => item.text() === 'references' && item.attributes('aria-level') === '2')
+    const treeItems = wrapper.findAll('[role="treeitem"]')
+    const nestedReferences = treeItems.find(item => item.text() === 'references' && item.attributes('aria-level') === '2')

     expect(nestedReferences).toBeDefined()
     await nestedReferences!.trigger('click')

-    const updatedItems = wrapper.findAll('[role="treeitem"]')
-    const rootReferences = updatedItems.find(item => item.text() === 'references' && item.attributes('aria-level') === '1')
-    const reopenedNestedReferences = updatedItems.find(item => item.text() === 'references' && item.attributes('aria-level') === '2')
+    const updatedTreeItems = wrapper.findAll('[role="treeitem"]')
+    const rootReferences = updatedTreeItems.find(item => item.text() === 'references' && item.attributes('aria-level') === '1')
+    const reopenedNestedReferences = updatedTreeItems.find(item => item.text() === 'references' && item.attributes('aria-level') === '2')

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/components/Tree.spec.ts` around lines 117 - 125, Rename the local test
variables that shadow the file-level fixture to avoid confusion: change the
local const items and const updatedItems in the test to distinct names (e.g.,
foundItems and updatedFoundItems) and update subsequent references
nestedReferences, rootReferences, and reopenedNestedReferences to use those new
names; ensure you only rename the local bindings used around the
wrapper.findAll('[role="treeitem"]') calls and the subsequent .find(...) lookups
so the top-level items fixture (defined at line 13) is no longer shadowed.

src/runtime/components/Tree.vue (1)

238-242: Note: Empty string from getKey triggers fallback.

The || operator means if a custom getKey returns "" (empty string), the logic falls through to getItemValueKey. This is likely the desired safety behavior but worth noting for documentation—if a user truly wants an empty-string key, this won't work.

If strict custom key handling is ever needed:

💡 Alternative using nullish coalescing

 function getItemKey<Item extends T[number]>(item: Item): string {
-  return props.getKey
-    ? props.getKey(item) || getItemValueKey(item) || getItemLabel(item)
+  return props.getKey
+    ? (props.getKey(item) ?? getItemValueKey(item) ?? getItemLabel(item)) || getItemLabel(item)
     : getItemValueKey(item) || getItemLabel(item)
 }

Given that empty-string keys are an unlikely edge case and the current behavior provides safer defaults, this is fine as-is.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/runtime/components/Tree.vue` around lines 238 - 242, The current
getItemKey function uses || which treats an empty string from props.getKey as
falsy and falls back to getItemValueKey/getItemLabel; if you need to preserve
explicit empty-string keys, change getItemKey to use nullish coalescing for the
custom key path (i.e., treat only null/undefined as missing) so
props.getKey(item) ?? getItemValueKey(item) ?? getItemLabel(item) is used;
update the logic in the getItemKey function (and ensure getKey, getItemValueKey,
getItemLabel references remain correct) and add a brief comment explaining why
?? is used.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/runtime/components/Tree.vue`:
- Around line 238-242: The current getItemKey function uses || which treats an
empty string from props.getKey as falsy and falls back to
getItemValueKey/getItemLabel; if you need to preserve explicit empty-string
keys, change getItemKey to use nullish coalescing for the custom key path (i.e.,
treat only null/undefined as missing) so props.getKey(item) ??
getItemValueKey(item) ?? getItemLabel(item) is used; update the logic in the
getItemKey function (and ensure getKey, getItemValueKey, getItemLabel references
remain correct) and add a brief comment explaining why ?? is used.

In `@test/components/Tree.spec.ts`:
- Around line 117-125: Rename the local test variables that shadow the
file-level fixture to avoid confusion: change the local const items and const
updatedItems in the test to distinct names (e.g., foundItems and
updatedFoundItems) and update subsequent references nestedReferences,
rootReferences, and reopenedNestedReferences to use those new names; ensure you
only rename the local bindings used around the
wrapper.findAll('[role="treeitem"]') calls and the subsequent .find(...) lookups
so the top-level items fixture (defined at line 13) is no longer shadowed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7e31d0ad-03dc-4cae-bc83-2fadff618713

📥 Commits

Reviewing files that changed from the base of the PR and between 9544d85 and b3c7afd.

📒 Files selected for processing (2)

• src/runtime/components/Tree.vue
• test/components/Tree.spec.ts

[pkg-pr-new]

npm i https://pkg.pr.new/@nuxt/ui@6189

commit: b3c7afd

[benjamincanac]

@sandros94 Do you remember why we chose not to have valueKey in Tree like other components? 🤔

[sandros94]

@sandros94 Do you remember why we chose not to have valueKey in Tree like other components? 🤔

TBH no, and checking when it was first being implemented I see that it was a period were I was not very active, so might be a simple oversight 😰

[howwohmm]

Looking at this Tree component fix, this is a solid approach to resolving the key collision issue. Here's my review:

• Mismatch between PR description and implementation: The description mentions building a WeakMap with index-based paths and using toRaw() for Vue proxies, but the actual implementation is much simpler—just preferring item.value over item.label for keys. The simpler approach is actually better, but the description should be updated to match what was actually implemented.

• Consider expanding supported value types: The getItemValueKey function only accepts string/number values, but boolean values could also make valid keys. Consider adding typeof value === 'boolean' to the type check, or document why only strings/numbers are supported if that's intentional.

• Test coverage looks comprehensive: The test effectively demonstrates the fix by creating two "references" items at different nesting levels with unique values, then verifying that toggling one doesn't affect the other's expansion state. This directly addresses the reported issue and provides good regression protection.

The core logic is sound and maintains backward compatibility perfectly. The fallback chain (getKey → value → label) makes sense and should resolve the duplicate key issues without breaking existing usage.