refactor(ui): fetch results on submit instead of on type#1107
refactor(ui): fetch results on submit instead of on type#1107MatteoGabriele wants to merge 20 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
…riele/npmx.dev into fix/search-reliability
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughReplaces the header-specific Header/SearchBox.vue with a new app/components/SearchBox.vue that exposes a public model 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/components/SearchBox.vue (1)
77-93: Remove inlinefocus-visibleutility from the<button>element.The button at line 79 uses
focus-visible:outline-accent/70, but project convention is to rely on the global rule inmain.cssfor buttons and selects. Based on learnings: "individual buttons or selects in Vue components should not rely on inline focus-visible utility classes likefocus-visible:outline-accent/70."♻️ Proposed fix
<button type="submit" - class="absolute hidden `@xs`:block group inset-ie-2.5 font-mono text-sm transition-[background-color,transform] duration-200 active:scale-95 focus-visible:outline-accent/70" + class="absolute hidden `@xs`:block group inset-ie-2.5 font-mono text-sm transition-[background-color,transform] duration-200 active:scale-95"app/components/AppHeader.vue (1)
33-42: Consider extracting duplicated search-navigation logic into a composable.
handleSearchSubmitis identical in bothindex.vueandAppHeader.vue. A shared composable (e.g.,useSearchNavigation) would reduce duplication and ensure consistent behaviour if the navigation logic changes.
|
would you check the e2e tests? they test a lot of functionality that has changed, so I expect they need to be updated.... |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/SearchBox.vue (1)
74-90: Remove inlinefocus-visibleutility from the submit button.The submit button applies
focus-visible:outline-accent/70as an inline utility class. The project applies focus-visible styling forbuttonandselectelements globally viamain.css. Per-element inline utilities on buttons should be avoided to keep styling consistent and maintainable.Proposed fix
<button type="submit" - class="absolute hidden `@xs`:block group inset-ie-2.5 font-mono text-sm transition-[background-color,transform] duration-200 active:scale-95 focus-visible:outline-accent/70" + class="absolute hidden `@xs`:block group inset-ie-2.5 font-mono text-sm transition-[background-color,transform] duration-200 active:scale-95"Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css… individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like focus-visible:outline-accent/70."
ghostdevv
left a comment
ghostdevv
left a comment
There was a problem hiding this comment.
I noticed that when I search for something that has an exact match, I press enter to trigger the search I see the results for half a second then I get taken to the package because of the exact match
Changes the update to occur on submit rather than on typing. This reduces requests to the registry API, minimizes router updates to the input value, which can cause issues like missing letters, and prevents empty responses when results are available.
it would be nice if it was throttled (as you're typing it searches every N seconds), but I understand that it may be tricky with the whole navigation between pages and such.
app/components/AppHeader.vue
Outdated
| await navigateTo({ | ||
| name: 'search', | ||
| query: { q: searchQuery.value }, | ||
| }) |
There was a problem hiding this comment.
what's the difference in nuxt between the router.push that was happening before, and this?
There was a problem hiding this comment.
In this case, I think either option would work.
Previously, this component had no route handling, so I copy-pasted the function from the homepage, as they serve the same purpose. I didn't check whether it was navigateTo or router.push, since they essentially perform the same action. I believe navigateTo is a helper function that can also be used on the server side.
MatteoGabriele
changed the title
|
@ghostdevv I've removed the "Enter to exact" functionality since we now use Enter to actually search. What do you think? |
|
I just wonder if we can't solve the search issue without sacrificing on enter to exact or the results updating as you type... like the registry requests is annoying but can be fine tuned/cached I think and is worth it for the user experience, and the other issues should be solvable 🤔 wdyt?
just as an alternative to debounce which has a nicer UX - but it only matters if we can fix the underlying problems |
|
@ghostdevv I have re-implemented an update-on-type feature, but instead of using a watcher, I am now debouncing the search input update so it is not linked to the route update. It seems to be working better, avoiding the weird update loop we had before. I also re-enabled the enter key functionality. Let me know what you think of this version. PS: The index still uses the Enter key to prevent accidental page change while typing, which could cause loss of focus or characters, or a disruptive experience. |
|
I'm actually finding issues again with the update-on-type. I will revert my last commit. |
This reverts commit 29a84e2.
| async function handleSearchSubmit() { | ||
| if (!searchQuery.value) { | ||
| return | ||
| } | ||
|
|
||
| async function search() { | ||
| const query = searchQuery.value.trim() | ||
| if (!query) return | ||
| await navigateTo({ | ||
| path: '/search', | ||
| query: query ? { q: query } : undefined, | ||
| name: 'search', | ||
| query: { q: searchQuery.value }, | ||
| }) |
There was a problem hiding this comment.
Trim whitespace-only queries before navigating.
Right now " " is treated as a valid query and will still navigate. Consider normalising to avoid empty/whitespace submissions (and mirror the same behaviour in the header submit handler for consistency).
Suggested tweak
async function handleSearchSubmit() {
- if (!searchQuery.value) {
+ const query = searchQuery.value.trim()
+ if (!query) {
return
}
await navigateTo({
name: 'search',
- query: { q: searchQuery.value },
+ query: { q: query },
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function handleSearchSubmit() { | |
| if (!searchQuery.value) { | |
| return | |
| } | |
| async function search() { | |
| const query = searchQuery.value.trim() | |
| if (!query) return | |
| await navigateTo({ | |
| path: '/search', | |
| query: query ? { q: query } : undefined, | |
| name: 'search', | |
| query: { q: searchQuery.value }, | |
| }) | |
| async function handleSearchSubmit() { | |
| const query = searchQuery.value.trim() | |
| if (!query) { | |
| return | |
| } | |
| await navigateTo({ | |
| name: 'search', | |
| query: { q: query }, | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/search.vue (1)
567-573:⚠️ Potential issue | 🟡 MinorRemove debug
console.logstatement.This appears to be a debug artifact that should be removed before merging.
Proposed fix
// Navigate if first result matches the query that was entered const firstResult = results[0] - // eslint-disable-next-line no-console - console.log('[search] watcher fired', { - pending: pendingEnterQuery.value, - firstResult: firstResult?.package.name, - }) if (firstResult?.package.name === pendingEnterQuery.value) {
| <SearchBox | ||
| v-if="!isOnHomePage" | ||
| ref="searchBoxRef" | ||
| :inputClass="isSearchExpanded ? 'w-full' : ''" | ||
| :class="{ 'max-w-md': !isSearchExpanded }" | ||
| class="max-w-sm" | ||
| compact | ||
| :model-value="searchQuery" | ||
| @update:model-value="updateSearchQuery" | ||
| @submit="handleSubmitSearch" | ||
| @focus="handleSearchFocus" | ||
| @blur="handleSearchBlur" | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SearchBox component
fd -t f "SearchBox" --extension vue
# Find AppHeader.vue to inspect the handlers
fd -t f "AppHeader" --extension vueRepository: npmx-dev/npmx.dev
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Read SearchBox component
echo "=== SearchBox.vue ==="
cat app/components/SearchBox.vue
echo -e "\n=== AppHeader.vue ==="
cat app/components/AppHeader.vueRepository: npmx-dev/npmx.dev
Length of output: 9164
🏁 Script executed:
#!/bin/bash
# Find usePackageSearchQuery composable to verify debouncing
fd -t f "usePackageSearchQuery" --extension ts --extension js
# Also search for where this composable is defined
rg "usePackageSearchQuery" -A 10 --type ts --type jsRepository: npmx-dev/npmx.dev
Length of output: 2015
🏁 Script executed:
#!/bin/bash
# Read the full usePackageSearchQuery composable
cat app/composables/usePackageSearchQuery.tsRepository: npmx-dev/npmx.dev
Length of output: 541
Use the value passed by the SearchBox submit event instead of reading the debounced state.
AppHeader's handleSubmitSearch ignores the input value emitted by SearchBox and reads directly from searchQuery.value, which is updated via a debounced 400ms function. If a user types quickly and presses Enter before the debounce fires, the navigation uses a stale query value. Update handleSubmitSearch(value: string) to accept and use the value from the submit event rather than reading from state.
| import { debounce } from 'perfect-debounce' | ||
|
|
||
| export function usePackageSearchQuery() { | ||
| const searchQuery = useState<string>('package_search_query', () => '') | ||
|
|
||
| const route = useRoute() | ||
| onMounted(() => { | ||
| if (route.query.q) { | ||
| searchQuery.value = normalizeSearchParam(route.query.q) | ||
| } | ||
| }) | ||
|
|
||
| const updateSearchQuery = debounce((newSearchQuery: string) => { | ||
| searchQuery.value = newSearchQuery | ||
| }, 400) | ||
|
|
||
| return { | ||
| searchQuery, | ||
| updateSearchQuery, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for normalizeSearchParam definition and usage
rg "normalizeSearchParam" --type=ts --type=vue -B2 -A2Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check the actual imports in usePackageSearchQuery.ts
cat app/composables/usePackageSearchQuery.tsRepository: npmx-dev/npmx.dev
Length of output: 541
🏁 Script executed:
# Check nuxt.config for auto-import configuration
fd "nuxt.config" -type f | xargs catRepository: npmx-dev/npmx.dev
Length of output: 231
🏁 Script executed:
# Search for normalizeSearchParam definition
rg "normalizeSearchParam" -A3 -B3Repository: npmx-dev/npmx.dev
Length of output: 5756
🏁 Script executed:
# Find nuxt.config files
find . -name "nuxt.config*" -type fRepository: npmx-dev/npmx.dev
Length of output: 99
🏁 Script executed:
# Check for utils/url.ts where normalizeSearchParam might be defined
find app -name "url.ts" -o -name "url.js"Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check nuxt.config for auto-import configuration
cat -n nuxt.config.tsRepository: npmx-dev/npmx.dev
Length of output: 9567
🏁 Script executed:
# Check the useStructuredFilters composable which also uses normalizeSearchParam
cat app/composables/useStructuredFilters.tsRepository: npmx-dev/npmx.dev
Length of output: 14869
Add missing import for normalizeSearchParam.
The composable uses normalizeSearchParam (line 8) without importing it. Add import { normalizeSearchParam } from '#shared/utils/url' at the top of the file.
|
@danielroe I made a change that might resolve the issue. |
app/pages/search.vue
Outdated
| onSuccess: data => { | ||
| router.replace({ | ||
| name: 'search', | ||
| query: { ...route.query, q: data.query }, | ||
| }) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect useNpmSearch for stale-response handling
find . -name "useNpmSearch.ts" -o -name "useNpmSearch.js" 2>/dev/nullRepository: npmx-dev/npmx.dev
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Check the context around lines 196-201 in search.vue
cat -n app/pages/search.vue | sed -n '180,220p'Repository: npmx-dev/npmx.dev
Length of output: 1470
🏁 Script executed:
#!/bin/bash
# Look for stale-response handling patterns (requestId, abort, cancellation)
rg -n "useNpmSearch|onSuccess|requestId|abort|cancel" app/ --type ts --type vue -A 3 -B 1 | head -100Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
#!/bin/bash
cat -n app/composables/npm/useNpmSearch.tsRepository: npmx-dev/npmx.dev
Length of output: 12899
Guard URL updates against out-of-order search responses.
onResponse updates the route with data.query. If requests overlap and responses arrive out of order, this can revert the URL to an older query. useNpmSearch invokes the callback before its stale-response check, so no built-in protection prevents this. Please add a guard.
🛠️ Optional guard
onResponse: data => {
+ if (data.query !== query.value) return
router.replace({
name: 'search',
query: { ...route.query, q: data.query },
})
},
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/composables/npm/useNpmSearch.ts (2)
136-148:⚠️ Potential issue | 🟡 MinorSame timing concern applies here, plus callback fires before
!pkgcheck.The callback is invoked before both the package existence check (line 138) and the stale query check (line 145). This means the URL could be updated even when no package exists or when the response is stale.
Consider moving the callback after both checks to ensure it only fires for valid, current responses.
Proposed fix
$npmApi<NpmDownloadCount>(`/downloads/point/last-week/${encodedName}`, { signal, }), ]) - opts.onResponse?.({ query: q }) - if (!pkg) { return emptySearchResponse } const result = packumentToSearchResult(pkg, downloads?.downloads) // If query changed/outdated, return empty search response if (q !== toValue(query)) { return emptySearchResponse } + opts.onResponse?.({ query: q }) + cache.value = {
173-179:⚠️ Potential issue | 🟡 MinorConsistent timing issue with
onResponsebefore stale check.Same concern as the other paths—moving the callback after the stale query check (line 176) would prevent URL updates for discarded responses.
Proposed fix
const { data: response, isStale } = await $npmRegistry<NpmSearchResponse>( `/-/v1/search?${params.toString()}`, { signal }, 60, ) - opts.onResponse?.({ query: q }) - // If query changed/outdated, return empty search response if (q !== toValue(query)) { return emptySearchResponse } + opts.onResponse?.({ query: q }) + cache.value = {
| opts.onResponse?.({ query: q }) | ||
|
|
||
| if (q !== toValue(query)) { | ||
| return emptySearchResponse | ||
| } |
There was a problem hiding this comment.
onResponse invoked before stale query check may cause URL desync.
The callback fires before verifying q !== toValue(query). If the user types quickly, the callback updates the URL with an outdated query, then the response is discarded as stale—leaving the URL out of sync with the actual search input.
Consider moving the callback invocation after the stale check, or rename to onResponseReceived if the current timing is intentional.
Proposed fix
const response = await searchAlgolia(q, {
size: opts.size ?? 25,
})
- opts.onResponse?.({ query: q })
-
if (q !== toValue(query)) {
return emptySearchResponse
}
+ opts.onResponse?.({ query: q })
+
isRateLimited.value = false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| opts.onResponse?.({ query: q }) | |
| if (q !== toValue(query)) { | |
| return emptySearchResponse | |
| } | |
| if (q !== toValue(query)) { | |
| return emptySearchResponse | |
| } | |
| opts.onResponse?.({ query: q }) | |
| isRateLimited.value = false |
4 participants
Uh oh!
There was an error while loading. Please reload this page.