Repository navigation
feat(Tree): add disabled for TreeItem - #2529
Conversation
📝 WalkthroughWalkthroughThis pull request adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/Tree/TreeItem.vue (1)
167-174:⚠️ Potential issue | 🟡 MinorExposed
handleToggleandhandleSelectbypass the disabled check.The handlers exposed via
defineExposedirectly callrootContext.onToggle()androotContext.onSelect()without checkingisDisabled. This allows programmatic access (via template refs) to toggle/select disabled items, which may be intentional for advanced use cases but could also be unexpected.If this is intentional (allowing programmatic override), consider documenting this behavior. If disabled items should remain non-interactive even programmatically, the handlers should check
isDisabled.🛡️ Proposed fix if disabled should be enforced programmatically
defineExpose({ isExpanded, isSelected, isIndeterminate, isDisabled, - handleToggle: () => rootContext.onToggle(props.value), - handleSelect: () => rootContext.onSelect(props.value), + handleToggle: () => { + if (!isDisabled.value) + rootContext.onToggle(props.value) + }, + handleSelect: () => { + if (!isDisabled.value) + rootContext.onSelect(props.value) + }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Tree/TreeItem.vue` around lines 167 - 174, The exposed handlers in defineExpose (handleToggle and handleSelect) call rootContext.onToggle/onSelect directly, allowing programmatic toggling/selecting of disabled items; update these handlers to first check isDisabled (use isDisabled.value if it's a ref/computed) and return/no-op when true before calling rootContext.onToggle(props.value) or rootContext.onSelect(props.value), ensuring disabled state is enforced programmatically; alternatively document intentional override behavior if you want to allow it.
🧹 Nitpick comments (1)
packages/core/src/Tree/Tree.test.ts (1)
381-387: Consider adding navigation test coverage.The current tests verify that disabled items don't respond to selection/toggle interactions. Consider adding a test that verifies keyboard navigation (ArrowUp/ArrowDown) skips over disabled items to reach the next enabled item, which would validate the
focusableintegration withRovingFocusItem.💡 Example test for navigation skipping disabled items
it('should skip disabled items during keyboard navigation', async () => { // Disable the middle item 'banana' const { items } = mountTree(['banana']) // Focus first item and navigate down ;(items()[0].element as HTMLElement).focus() await items()[0].trigger('keydown', { key: kbd.ARROW_DOWN }) // Should skip 'banana' and focus 'cherry' expect(document.activeElement).toBe(items()[2].element) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Tree/Tree.test.ts` around lines 381 - 387, Add a new test in Tree.test.ts that verifies keyboard navigation skips disabled items by using the existing mountTree helper and items() accessor: call mountTree(['banana']) to disable the middle item, focus the first item via (items()[0].element as HTMLElement).focus(), trigger a keydown on items()[0] with { key: kbd.ARROW_DOWN }, and assert document.activeElement === items()[2].element; this ensures the component's `focusable` behavior (RovingFocusItem integration) correctly skips the disabled node during ArrowUp/ArrowDown navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/Tree/TreeItem.vue`:
- Around line 167-174: The exposed handlers in defineExpose (handleToggle and
handleSelect) call rootContext.onToggle/onSelect directly, allowing programmatic
toggling/selecting of disabled items; update these handlers to first check
isDisabled (use isDisabled.value if it's a ref/computed) and return/no-op when
true before calling rootContext.onToggle(props.value) or
rootContext.onSelect(props.value), ensuring disabled state is enforced
programmatically; alternatively document intentional override behavior if you
want to allow it.
---
Nitpick comments:
In `@packages/core/src/Tree/Tree.test.ts`:
- Around line 381-387: Add a new test in Tree.test.ts that verifies keyboard
navigation skips disabled items by using the existing mountTree helper and
items() accessor: call mountTree(['banana']) to disable the middle item, focus
the first item via (items()[0].element as HTMLElement).focus(), trigger a
keydown on items()[0] with { key: kbd.ARROW_DOWN }, and assert
document.activeElement === items()[2].element; this ensures the component's
`focusable` behavior (RovingFocusItem integration) correctly skips the disabled
node during ArrowUp/ArrowDown navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8c4af0f-adfe-45fc-b8e3-2fe83ae2ef5a
📒 Files selected for processing (4)
docs/content/meta/TreeItem.mdpackages/core/src/Tree/Tree.test.tspackages/core/src/Tree/TreeItem.vuepackages/core/src/Tree/TreeVirtualizer.vue
c89b894 to
06bf624
Compare
🔗 Linked issue
resolves #2528
❓ Type of change
📚 Description
📸 Screenshots (if appropriate)
📝 Checklist
Summary by CodeRabbit