-
Notifications
You must be signed in to change notification settings - Fork 119
fix(suggestion): empty array compatibility for items #455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdded a guard condition in the watch handler for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5–10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
✅ Deploy Preview for antd-design-x-vue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/suggestion/useActive.ts (1)
46-57: Emptyitemscan still cause a runtime error via keyboard navigationEven with the new guard on the watcher, if
openistrueanditemsis an empty array, pressing ArrowUp/ArrowDown will hitoffsetRow, whereitemCountcan be0:const itemCount = currentItems.length; // ... const nextItem = currentItems[(currentRowIndex + offset + itemCount) % itemCount]; setActivePaths([...activePaths.value.slice(0, currentColIndex - 1), nextItem.value]);When
itemCount === 0, this computes(... % 0), yieldingNaN, andnextItembecomesundefined, sonextItem.valuethrows. To fully support emptyitems, you can early-return when there are no items:const offsetRow = (offset: number) => { const currentColIndex = activePaths.value.length || 1; const currentItems = getItems(currentColIndex); const itemCount = currentItems.length; if (!itemCount) { return; } const currentRowIndex = currentItems.findIndex( (item) => item.value === activePaths.value[currentColIndex - 1], ); const nextItem = currentItems[(currentRowIndex + offset + itemCount) % itemCount]; setActivePaths([...activePaths.value.slice(0, currentColIndex - 1), nextItem.value]); };This is a pre-existing issue, but it’s closely related to the empty-array compatibility you’re addressing here.
🧹 Nitpick comments (1)
src/suggestion/useActive.ts (1)
123-128: Watcher guard correctly prevents crashes; consider small cleanupThe new condition achieves the goal of avoiding a crash when
itemsisnull/undefinedor an empty array. To simplify and avoid multiple evaluations, you could cacheopenanditemsonce inside the watcher:- watch(() => toValue(open), () => { - // 确保 items 是一个数组且至少有一个元素 - if (toValue(open) && Array.isArray(toValue(items)) && toValue(items).length > 0) { - setActivePaths([toValue(items)[0].value]); - } - }, { immediate: true }); + watch(() => toValue(open), () => { + const openVal = toValue(open); + const itemsVal = toValue(items); + + // 确保 items 是一个数组且至少有一个元素 + if (openVal && Array.isArray(itemsVal) && itemsVal.length > 0) { + setActivePaths([itemsVal[0].value]); + } + }, { immediate: true });This keeps the behavior identical while making the guard easier to read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/suggestion/useActive.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules - antd-design-x-vue
- GitHub Check: Header rules - antd-design-x-vue
- GitHub Check: Pages changed - antd-design-x-vue
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.