perf: speed up algolia 🚀 + split out search suggestion logic #1271
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 pull request introduces a suggestions feature to the search functionality by consolidating suggestion-related logic from the search page into the unified search composable. Changes include adding provider-aware debounce timing in SearchBox and index components, expanding search utility exports with validation and intent-parsing functions, introducing multi-search capability with existence checks in Algolia integration, and enhancing the useSearch composable with optional suggestions configuration, caching mechanisms, and parallelised existence checks. The search page is refactored to rely on the composable's suggestion data rather than managing local validation logic. Possibly related PRs
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 inconclusive)
✏️ 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: 1
🧹 Nitpick comments (5)
app/composables/npm/search-utils.ts (1)
40-44: Consider extending validation to match npm naming rules more precisely.The current regex
/^[\w-]+$/doesn't allow dots (.), but npm package names can contain dots (e.g.,lodash.get). Additionally, npm enforces lowercase names, but the regex allows uppercase via\w.That said, this validation is used for suggestion intent parsing, not for publishing validation, so being slightly stricter may be acceptable depending on use case.
♻️ Optional: More accurate npm name validation
export function isValidNpmName(name: string): boolean { if (!name || name.length === 0 || name.length > 214) return false if (!/^[a-z0-9]/i.test(name)) return false - return /^[\w-]+$/.test(name) + // npm names: lowercase alphanumeric, hyphens, underscores, and dots + return /^[a-z0-9._-]+$/i.test(name) }app/composables/npm/useAlgoliaSearch.ts (1)
306-317: Same escaping concern forobjectIDfilter.The
checkPackagevalue is also used directly in a filter and could contain special characters (e.g., scoped packages like@scope/name).♻️ Proposed fix
if (checks?.checkPackage) { requests.push({ indexName, query: '', - filters: `objectID:${checks.checkPackage}`, + filters: `objectID:${escapeFilterValue(checks.checkPackage)}`, length: 1, analyticsTags: ['npmx.dev'], attributesToRetrieve: EXISTENCE_CHECK_ATTRS, attributesToHighlight: [], }) }app/composables/npm/useSearch.ts (1)
325-408: Consider extracting the npm suggestion validation into a separate helper.The
validateSuggestionsNpmfunction is quite long (~85 lines) and handles multiple concerns: package availability checking, org/user existence checks, and result assembly. Extracting sub-helpers would improve readability.That said, the logic is correct and the race condition handling via
requestIdis well implemented.app/pages/search.vue (2)
581-587: Inlinefocus-visiblestyling on button should rely on global CSS.Based on learnings, focus-visible styling for buttons is implemented globally in
main.css. The inlinefocus-visible:outline-accent/70class 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".
691-697: Samefocus-visiblestyling concern on this button.This button also has inline
focus-visible:outline-accent/70which should be removed per the global styling guideline.♻️ 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 and selects is applied globally via main.css".
| async function checkOrgExists(name: string): Promise<boolean> { | ||
| try { | ||
| const scopePrefix = `@${name.toLowerCase()}/` | ||
| const response = await $fetch<{ | ||
| total: number | ||
| objects: Array<{ package: { name: string } }> | ||
| }>(`${NPM_REGISTRY}/-/v1/search`, { query: { text: `@${name}`, size: 5 } }) | ||
| return response.objects.some(obj => obj.package.name.toLowerCase().startsWith(scopePrefix)) | ||
| } catch { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and read the useNpmSearch.ts file
fd "useNpmSearch.ts" --type fRepository: npmx-dev/npmx.dev
Length of output: 96
🏁 Script executed:
# Check the imports and the referenced lines in the file
cat -n app/composables/npm/useNpmSearch.ts | head -40Repository: npmx-dev/npmx.dev
Length of output: 1464
🏁 Script executed:
# Search for NPM_REGISTRY in the entire composables directory
rg "NPM_REGISTRY" app/composables/npm/ -nRepository: npmx-dev/npmx.dev
Length of output: 304
🏁 Script executed:
# Check if NPM_REGISTRY is defined in the constants file
rg "NPM_REGISTRY" app --type=ts | head -20Repository: npmx-dev/npmx.dev
Length of output: 682
🏁 Script executed:
# Let me check if NPM_REGISTRY might be defined elsewhere in the file or if it's supposed to be a global
cat -n app/composables/npm/useNpmSearch.ts | tail -60Repository: npmx-dev/npmx.dev
Length of output: 1886
🏁 Script executed:
# Check the full context of where $npmRegistry is used in the composable
rg -n '\$npmRegistry|NPM_REGISTRY' app/composables/npm/useNpmSearch.ts -A2 -B2Repository: npmx-dev/npmx.dev
Length of output: 840
Missing import for NPM_REGISTRY constant.
The NPM_REGISTRY constant is used on lines 15 and 24 but is not imported. According to other files in the codebase (e.g., app/utils/package-name.ts), it's exported from #shared/utils/constants. This will cause a ReferenceError at runtime when either helper function is called.
Proposed fix to add the missing import
import type { NpmSearchResponse, PackageMetaResponse } from '#shared/types'
import { emptySearchResponse, metaToSearchResult } from './search-utils'
+import { NPM_REGISTRY } from '#shared/utils/constants'📝 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 checkOrgExists(name: string): Promise<boolean> { | |
| try { | |
| const scopePrefix = `@${name.toLowerCase()}/` | |
| const response = await $fetch<{ | |
| total: number | |
| objects: Array<{ package: { name: string } }> | |
| }>(`${NPM_REGISTRY}/-/v1/search`, { query: { text: `@${name}`, size: 5 } }) | |
| return response.objects.some(obj => obj.package.name.toLowerCase().startsWith(scopePrefix)) | |
| } catch { | |
| return false | |
| } | |
| } | |
| import type { NpmSearchResponse, PackageMetaResponse } from '#shared/types' | |
| import { emptySearchResponse, metaToSearchResult } from './search-utils' | |
| import { NPM_REGISTRY } from '#shared/utils/constants' |
2 participants
No description provided.