fix: normalize user and org names to lowercase in search and routing#1519
fix: normalize user and org names to lowercase in search and routing#1519yshashi wants to merge 1 commit intonpmx-dev:mainfrom
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! |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughRoute and name handling were normalised to lowercase across the app: search suggestion routes now pass names as Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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/npm/useSearch.ts (1)
371-418:⚠️ Potential issue | 🟠 MajorPass lowercased names to NPM existence checks.
The functions
checkOrgNpm(name)andcheckUserNpm(name)are called with the original casing, but their results are cached withlowerNamekeys. Both functions send queries to the NPM registry using the original case (line 15 incheckOrgExistssends@${name}, and line 25 incheckUserExistssendsmaintainer:${name}). If the registry is case-sensitive, mixed-case input could produce false negatives—for example, a search for 'Node-Red' might fail, but the result gets cached asorg:node-red, causing a subsequent search for 'node-red' to incorrectly use the cached failure. PasslowerNameto both functions to ensure consistent API queries and alignment with cache keys.Suggested fix
- checkOrgNpm(name) + checkOrgNpm(lowerName) ... - checkUserNpm(name) + checkUserNpm(lowerName)
🧹 Nitpick comments (1)
server/api/registry/org/[org]/packages.get.ts (1)
19-59: Normalise the cache key to avoid duplicate entries.With the handler lowercasing
org, the cache key still uses the raw route param, so/org/Angularand/org/angularwon’t share cache entries. Consider lowercasing ingetKeyas well.Suggested fix
getKey: event => { - const org = getRouterParam(event, 'org') ?? '' + const org = getRouterParam(event, 'org')?.toLowerCase() ?? '' return `org-packages:v1:${org}` },
9274f07 to
761e7ae
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)
server/api/registry/org/[org]/packages.get.ts (1)
56-59:⚠️ Potential issue | 🟠 MajorCache key does not normalise
orgto lowercase, causing cache fragmentation.The handler normalises
orgto lowercase at line 19, but the cachegetKeyfunction uses the raw parameter. Requests for/org/Angular/packagesand/org/angular/packageswill produce different cache keys (org-packages:v1:Angularvsorg-packages:v1:angular) yet fetch identical data from the npm registry.🔧 Proposed fix to normalise cache key
getKey: event => { - const org = getRouterParam(event, 'org') ?? '' - return `org-packages:v1:${org}` + const org = getRouterParam(event, 'org')?.toLowerCase() ?? '' + return `org-packages:v1:${org}` },
🧹 Nitpick comments (1)
test/unit/search-case-normalization.spec.ts (1)
37-45: Consider removing this test block — it only asserts JavaScript'stoLowerCase()behaviour.The "Case normalization expectations" describe block doesn't test any application logic; it merely confirms that
'AnGuLAR'.toLowerCase()equals'angular'. If the intent is to document that consumers must normalise, a code comment inparseSuggestionIntentoruseSearch.tswould be clearer.Alternatively, consider testing the actual normalisation in
useSearch.tsby mocking the composable and verifying that suggestions contain lowercased names.
761e7ae to
0d456e1
Compare
Changes
useSearch.tsSearchSuggestionCard.vueTesting
/org/angular(lowercase)Fixes: #1507