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 21 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! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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)
No actionable comments were generated in the recent review. 🎉 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.
|
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) {
| 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.
|
@ghostdevv I made a change that might resolve the issue. The code might needs some cleaning, but I wanted to hear your thoughts first. I'm using a shared state variable, updating the URL only on submit or when the promise resolves. The input field is no longer connected to the URL preventing update loops by syncing only once on landing. wdyt? |
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 |
|
I'm going to close this PR because the merge conflicts are getting larger and larger, and I might have found a way to fix the update-on-type issue. I also don't want to tie this decision to the input component refactor. |
4 participants
Uh oh!
There was an error while loading. Please reload this page.