feat: add pagination for item selector#1422
Conversation
WalkthroughThe Item Selector component now supports pagination of command list results through a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Suggested labels
Suggested reviewers
Security RecommendationsWhen reviewing pagination logic, please verify:
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/components/Item/Selector.vue (3)
202-203: Minor type simplification opportunity.
pageInputis typed asref<string | number>but is consistently set to a string viaString()throughout the code. Since HTML number inputs return strings viav-model, the type could be simplified toref<string>("1")for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Item/Selector.vue` around lines 202 - 203, Change pageInput from ref<string | number>("1") to ref<string>("1") to reflect that HTML number inputs and existing String() calls always produce strings; update any places that consume pageInput as a number (e.g., pagination handlers or uses alongside currentPage) to explicitly parseInt(pageInput.value, 10) or use Number(...) before numeric operations, and keep currentPage as ref(1) unchanged. Use the symbols pageInput and currentPage to locate and adjust the related code in Selector.vue.
177-194: Consider validatingpageSizeto be positive.If a caller passes
pageSize: 0or a negative value,pageCountbecomesInfinityorNaN, andfindPageForSelectedKeywould produce incorrect results due to division by zero. While unlikely in practice, a defensive check could prevent subtle bugs.♻️ Optional: Add runtime validation
+ const effectivePageSize = computed(() => Math.max(1, props.pageSize)); + - const pageCount = computed(() => Math.ceil(filtered.value.length / props.pageSize)); + const pageCount = computed(() => Math.ceil(filtered.value.length / effectivePageSize.value)); // ... and use effectivePageSize.value elsewhere instead of props.pageSize🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Item/Selector.vue` around lines 177 - 194, Validate and guard the pageSize prop to ensure it's a positive integer before it's used (currently set via withDefaults on the props), because zero or negative values make pageCount Infinity/NaN and break findPageForSelectedKey; update the prop normalization where pageSize is defined (the withDefaults/defineProps block) to coerce or clamp incoming pageSize to a minimum of 1 (or throw a clear error) and ensure any local calculations that read pageSize (e.g., pageCount computation and findPageForSelectedKey) use the normalized value.
344-357: The blur loop may be more aggressive than necessary.
event.preventDefault()already prevents focus transfer from pagination buttons. The subsequent loop that blurs all inputs in the parent element (including the search input) seems redundant and could cause unexpected UX where the search input loses focus when navigating pages.If the intent is specifically to blur the page number input after interaction, consider targeting it more precisely.
♻️ Simplify to rely on preventDefault alone
function handlePaginationToolbarPointerDown(event: PointerEvent) { - const toolbar = event.currentTarget as HTMLElement | null; const target = event.target as HTMLElement | null; if (target?.closest("input")) { return; } event.preventDefault(); - const inputElements = toolbar?.parentElement?.querySelectorAll<HTMLInputElement>("input"); - inputElements?.forEach(inputElement => { - inputElement.blur(); - }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Item/Selector.vue` around lines 344 - 357, The blur loop in handlePaginationToolbarPointerDown is too aggressive and unnecessary; remove the blanket inputElements.forEach(...) blur logic and either rely solely on event.preventDefault() to stop focus changes or target and blur only the specific page-number input (e.g., locate it via a dedicated selector or class such as a page-input or name="page" using toolbar.parentElement?.querySelector) so the search input doesn't lose focus unintentionally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/components/Item/Selector.vue`:
- Around line 202-203: Change pageInput from ref<string | number>("1") to
ref<string>("1") to reflect that HTML number inputs and existing String() calls
always produce strings; update any places that consume pageInput as a number
(e.g., pagination handlers or uses alongside currentPage) to explicitly
parseInt(pageInput.value, 10) or use Number(...) before numeric operations, and
keep currentPage as ref(1) unchanged. Use the symbols pageInput and currentPage
to locate and adjust the related code in Selector.vue.
- Around line 177-194: Validate and guard the pageSize prop to ensure it's a
positive integer before it's used (currently set via withDefaults on the props),
because zero or negative values make pageCount Infinity/NaN and break
findPageForSelectedKey; update the prop normalization where pageSize is defined
(the withDefaults/defineProps block) to coerce or clamp incoming pageSize to a
minimum of 1 (or throw a clear error) and ensure any local calculations that
read pageSize (e.g., pageCount computation and findPageForSelectedKey) use the
normalized value.
- Around line 344-357: The blur loop in handlePaginationToolbarPointerDown is
too aggressive and unnecessary; remove the blanket inputElements.forEach(...)
blur logic and either rely solely on event.preventDefault() to stop focus
changes or target and blur only the specific page-number input (e.g., locate it
via a dedicated selector or class such as a page-input or name="page" using
toolbar.parentElement?.querySelector) so the search input doesn't lose focus
unintentionally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b0967c5c-5624-4a90-b3f0-c2d1a400cb51
📒 Files selected for processing (2)
frontend/components/Item/Selector.vuefrontend/locales/en.json
What type of PR is this?
What this PR does / why we need it:
This PR fixes a performance issue caused by rendering too many items in the item selector popover by adding a paginated UI.
frontend/components/Item/Selector.vue: Add pagination for the item selector, using a single request and handling pagination on the frontend to stay closer to the original behavior. Some item list paginations already work this way.frontend/locales/en.json: adds English localization strings foritems.first_pageanditems.last_page.Which issue(s) this PR fixes:
Fixes #1333
Special notes for your reviewer:
I'm a bit confused by the existed i18n keys and translations for the pagination controls' aria labels (
items.prev_page,items.next_page,items.first, anditems.last), so I checked the commit history.These keys were introduced in 0a4c5fb, then become unused in cbaf483 due to a component change. After that,
items.firstanditems.lastwere translated to different meanings across languages. For example,items.lasthas been translated as "last", "last item", "last page", or even "last name".In this change, I reused
items.prev_pageanditems.next_page. However, for translation accuracy and clearer semantics, I addeditems.first_pageanditems.last_pageinstead of reusing the existingitems.firstanditems.last.If needed, I believe
items.firstanditems.lastcould be safely removed.Summary by CodeRabbit
Release Notes