Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis PR introduces a centralized useGlobalSearch composable that consolidates global search state, provider resolution, and URL synchronization (with debounced update and a flush helper). It removes the previous useGlobalSearchQuery composable and replaces per-component URL-sync logic across SearchBox.vue, Keywords.vue, index.vue and search.vue to consume the new composable (model and provider). SearchBox.vue now exposes a focus() method and wires an input ref; Keywords.vue sets the global model when a keyword is clicked. useStructuredFilters gains an optional searchQueryModel Ref to keep external query state in sync. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| const updateUrlQueryImpl = (value: string, provider: 'npm' | 'algolia') => { | ||
| const isSameQuery = route.query.q === value && route.query.p === provider | ||
| // Don't navigate away from pages that use ?q for local filtering | ||
| if (pagesWithLocalFilter.has(route.name as string) || isSameQuery) { | ||
| return |
There was a problem hiding this comment.
Preserve route-level provider overrides when updating the URL.
Right now the setter passes searchProvider.value (settings) into URL updates. If a user arrives with p=npm while their settings default to Algolia, the first keystroke drops p and flips providers. Use the effective provider (route + settings) and normalise current route values for isSameQuery.
💡 Suggested fix
- const isSameQuery = route.query.q === value && route.query.p === provider
+ const currentQuery = normalizeSearchParam(route.query.q)
+ const currentProvider = normalizeSearchParam(route.query.p) === 'npm' ? 'npm' : 'algolia'
+ const isSameQuery = currentQuery === value && currentProvider === provider
@@
- if (!updateUrlQuery.isPending()) {
- updateUrlQueryImpl(value, searchProvider.value)
- }
- updateUrlQuery(value, searchProvider.value)
+ const effectiveProvider = searchProviderValue.value
+ if (!updateUrlQuery.isPending()) {
+ updateUrlQueryImpl(value, effectiveProvider)
+ }
+ updateUrlQuery(value, effectiveProvider)Also applies to: 64-71
| function flushUpdateUrlQuery() { | ||
| updateUrlQuery.flush() | ||
| } |
There was a problem hiding this comment.
Submitting a stable query can no‑op.
flushUpdateUrlQuery() only flushes pending debounced updates. If the user submits an already‑settled query (for example, the prefilled header search), nothing navigates. Consider falling back to an immediate update when no debounce is pending.
💡 Suggested fix
function flushUpdateUrlQuery() {
- updateUrlQuery.flush()
+ if (updateUrlQuery.isPending()) {
+ updateUrlQuery.flush()
+ return
+ }
+ updateUrlQueryImpl(searchQuery.value, searchProviderValue.value)
}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/pages/search.vue (2)
580-586: Remove inlinefocus-visibleutility on button element.Per project guidelines, focus-visible styling for buttons is applied globally via
main.css. The inlinefocus-visible:outline-accent/70class here should be removed to maintain consistency.♻️ Proposed fix
<button type="button" - class="shrink-0 px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md motion-safe:transition-colors motion-safe:duration-200 hover:bg-fg/90 focus-visible:outline-accent/70" + class="shrink-0 px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md motion-safe:transition-colors motion-safe:duration-200 hover:bg-fg/90" `@click`="claimPackageModalRef?.open()" >Based on learnings: "In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css… Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates."
693-699: Remove inlinefocus-visibleutility on button element.Same as above—rely on the global
button:focus-visiblerule rather than the inline utility class.♻️ Proposed fix
<button type="button" - class="px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-colors duration-200 hover:bg-fg/90 focus-visible:outline-accent/70" + class="px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-colors duration-200 hover:bg-fg/90" `@click`="claimPackageModalRef?.open()" >Based on learnings: focus-visible styling for buttons should use the global CSS rule, not inline utility classes.
alexdln
force-pushed
the
perf/search-improvements
branch
from
ff49d2d to
ee53118
Compare
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)
app/composables/useStructuredFilters.ts (1)
401-417:⚠️ Potential issue | 🟠 MajorAvoid dropping in-flight query text when adding/removing keywords.
With debounced URL updates,searchQuery.valuecan lag behind the active input. BuildingnewQfrom it can overwrite recent typing. Prefer the external model when provided and keep the local ref in sync.Proposed fix
function addKeyword(keyword: string) { if (!filters.value.keywords.includes(keyword)) { filters.value.keywords = [...filters.value.keywords, keyword] - const newQ = searchQuery.value - ? `${searchQuery.value.trim()} keyword:${keyword}` - : `keyword:${keyword}` + const baseQuery = (searchQueryModel?.value ?? searchQuery.value).trim() + const newQ = baseQuery ? `${baseQuery} keyword:${keyword}` : `keyword:${keyword}` router.replace({ query: { ...route.query, q: newQ } }) if (searchQueryModel) searchQueryModel.value = newQ + searchQuery.value = newQ } } function removeKeyword(keyword: string) { filters.value.keywords = filters.value.keywords.filter(k => k !== keyword) - const newQ = searchQuery.value.replace(new RegExp(`keyword:${keyword}($| )`, 'g'), '').trim() + const baseQuery = searchQueryModel?.value ?? searchQuery.value + const newQ = baseQuery.replace(new RegExp(`keyword:${keyword}($| )`, 'g'), '').trim() router.replace({ query: { ...route.query, q: newQ || undefined } }) if (searchQueryModel) searchQueryModel.value = newQ + searchQuery.value = newQ }
2 participants
Completely separated the search process from routing and created a single manager for working with global search, which is responsible for the query and updates the URL every 250ms.
Since there is no longer a binding from query to input, I made a leading debounce, which will allow the user to immediately direct to the search, rather than waiting 80-250ms
Also fixed a few navigation issues - keywords in the package weren't working and the global search field was being updated unnecessarily on the compare page.