fix: filter out security holding packages from Algolia results#2026
fix: filter out security holding packages from Algolia results#2026
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe search page's visibleResults computation now excludes packages flagged as security-held (checks Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@shuuji3 Maybe it's better to check only |
|
@alexdln Right, probably that's simpler and rubust. I thought repository information might not exist, but I now thing it's unlikely in this case. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/types/npm-registry.ts (1)
316-328: Consider making non-essential fields optional for robustness against API variations.The
NpmSearchRepositoryinterface currently marks all fields as required, but these fields originate from the Algolia API where responses may be incomplete. Additionally, the fieldsproject,user,host,path, andbranchare not accessed anywhere in the codebase, suggesting they are either unused or conditionally present in API responses.Making non-essential fields optional aligns with defensive coding practices for external API types and prevents potential type mismatches if Algolia returns partial repository data. The
type: 'git'literal may also be overly restrictive should npm support additional VCS types in future.♻️ Suggested change
export interface NpmSearchRepository { - type: 'git' + type?: string url: string - project: string - user: string - host: string - path: string - branch: string + project?: string + user?: string + host?: string + path?: string + branch?: string }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b673ac2a-7dfa-4a1e-99d0-9f28e7f33e93
📒 Files selected for processing (2)
app/pages/search.vueshared/types/npm-registry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/pages/search.vue
serhalp
changed the title
|
@shuuji3 🤔 hmm, it doesn't seem to be filtering them out. in any case, maybe we should add test coverage for this? |
…out-security-holding-packages
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/useAlgoliaSearch.ts (1)
37-54: 🛠️ Refactor suggestion | 🟠 MajorMake
isSecurityHeldoptional at the Algolia boundary for defensive typing.
NpmSearchPackage.isSecurityHeldis optional, and external API boundaries should be typed defensively. ChangeAlgoliaHit.isSecurityHeldto optional, but preserve the direct assignment in the mapping to maintain bothtrueandfalsevalues as verified by tests.♻️ Corrected adjustment
interface AlgoliaHit { @@ - isSecurityHeld: boolean + isSecurityHeld?: boolean }Keep the mapping as:
isSecurityHeld: hit.isSecurityHeld🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/npm/useAlgoliaSearch.ts` around lines 37 - 54, The Algolia interface should be defensive: change the AlgoliaHit interface's isSecurityHeld property to optional (isSecurityHeld?: boolean) so it matches upstream NpmSearchPackage typing, and keep the existing mapping that assigns the value directly (e.g., isSecurityHeld: hit.isSecurityHeld) so both true and false values are preserved by the conversion logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/composables/npm/useAlgoliaSearch.ts`:
- Around line 37-54: The Algolia interface should be defensive: change the
AlgoliaHit interface's isSecurityHeld property to optional (isSecurityHeld?:
boolean) so it matches upstream NpmSearchPackage typing, and keep the existing
mapping that assigns the value directly (e.g., isSecurityHeld:
hit.isSecurityHeld) so both true and false values are preserved by the
conversion logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 996c8a45-6d56-4a83-bc72-973d9b8d9d23
📒 Files selected for processing (2)
app/composables/npm/useAlgoliaSearch.tsapp/pages/search.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- app/pages/search.vue
|
I found that Algoria API actually has the exact attribute for the security holding pakcages called I also added tests for |
| let objects = raw.objects | ||
|
|
||
| // Filter out "Security holding package" packages taken down by npm registry | ||
| objects = objects.filter(r => !r.package.isSecurityHeld) |
There was a problem hiding this comment.
💭 Wouldn't it be preferable to implement this filtering in the useAlgoliaSearch or useGlobalSearch composable? Otherwise we'd still be showing security holding packages in other places we use search today (compare page typeahead) and in the future.
There was a problem hiding this comment.
I'm going to merge this since it's a somewhat important fix, but I think this is worth considering as a follow-up refactor!
There was a problem hiding this comment.
@serhalp Thanks, I think useAlgoliaSearch would be a proper location to add filter since the npm API returns the pre-filtered package list so we can make both consistent. I'll create a follow-up PR when I found time later 🙏🏻
3 participants
🔗 Linked issue
resolves #2002
🧭 Context
Algoria search returns packages already takendown by npm registory and marked as "Security holding package".
📚 Description
Filter out packages where the package's version is
0.0.1-securityAND its description issecurity holding package.You can compare:
Example returned value from algoria search API (from network tab):
{ "results": [ { "hits": [ { "name": "dowload_ebok_grundkurs_kunstliche_intelligenz_by_wolfgang_ertel_r9sfy", "downloadsLast30Days": 0, "downloadsRatio": 0, "popular": false, "version": "0.0.1-security", "description": "security holding package", "repository": { "type": "git", "url": "npm/security-holder", "project": "security-holder", "user": "npm", "host": "github.com", "path": "", "branch": "master" }, "deprecated": false, "isDeprecated": false, "homepage": null, "license": null, "keywords": [], "modified": 1758520468387, "owners": [], "objectID": "dowload_ebok_grundkurs_kunstliche_intelligenz_by_wolfgang_ertel_r9sfy" }, { "name": "dowload_ebok_farouche_atalante_by_emilie_druilhe_nx2bj", "downloadsLast30Days": 0, "downloadsRatio": 0, "popular": false, "version": "0.0.1-security", "description": "security holding package", "repository": { "type": "git", "url": "npm/security-holder", "project": "security-holder", "user": "npm", "host": "github.com", "path": "", "branch": "master" }, "deprecated": false, "isDeprecated": false, "homepage": null, "license": null, "keywords": [], "modified": 1725351874194, "owners": [], "objectID": "dowload_ebok_farouche_atalante_by_emilie_druilhe_nx2bj" }, { "name": "dowload_ebok_everything_in_between_a_rocker_romance_by_melissa_toppen_fp7ge", "downloadsLast30Days": 0, "downloadsRatio": 0, "popular": false, "version": "0.0.1-security", "description": "security holding package", "repository": { "type": "git", "url": "npm/security-holder", "project": "security-holder", "user": "npm", "host": "github.com", "path": "", "branch": "master" }, "deprecated": false, "isDeprecated": false, "homepage": null, "license": null, "keywords": [], "modified": 1725351869604, "owners": [], "objectID": "dowload_ebok_everything_in_between_a_rocker_romance_by_melissa_toppen_fp7ge" }, { "name": "dowload_ebok_englens_spil_by_carlos_ruiz_zafon_iben_hasselbalch_lqvq8", "downloadsLast30Days": 0, "downloadsRatio": 0, "popular": false, "version": "0.0.1-security", "description": "security holding package", "repository": { "type": "git", "url": "npm/security-holder", "project": "security-holder", "user": "npm", "host": "github.com", "path": "", "branch": "master" }, "deprecated": false, "isDeprecated": false, "homepage": null, "license": null, "keywords": [], "modified": 1745588033919, "owners": [], "objectID": "dowload_ebok_englens_spil_by_carlos_ruiz_zafon_iben_hasselbalch_lqvq8" }, { "name": "dowload_ebok_before_dawn_vampire_fallen_book_1_by_morgan_rice_fjwc4", "downloadsLast30Days": 0, "downloadsRatio": 0, "popular": false, "version": "0.0.1-security", "description": "security holding package", "repository": { "type": "git", "url": "npm/security-holder", "project": "security-holder", "user": "npm", "host": "github.com", "path": "", "branch": "master" }, "deprecated": false, "isDeprecated": false, "homepage": null, "license": null, "keywords": [], "modified": 1762128250838, "owners": [], "objectID": "dowload_ebok_before_dawn_vampire_fallen_book_1_by_morgan_rice_fjwc4" }, { "name": "dowload_ebok_a_guerra_de_hitler_e_o_horror_do_holocausto_by_scott_s_f_meaker_k644u", "downloadsLast30Days": 0, "downloadsRatio": 0, "popular": false, "version": "0.0.1-security", "description": "security holding package", "repository": { "type": "git", "url": "npm/security-holder", "project": "security-holder", "user": "npm", "host": "github.com", "path": "", "branch": "master" }, "deprecated": false, "isDeprecated": false, "homepage": null, "license": null, "keywords": [], "modified": 1762128244458, "owners": [], "objectID": "dowload_ebok_a_guerra_de_hitler_e_o_horror_do_holocausto_by_scott_s_f_meaker_k644u" }, { "name": "dowload_ebok_a_bela_e_a_fera_by_elizabeth_rudnick_evan_spiliotopoulos_stephen_ch_9ooey", "downloadsLast30Days": 0, "downloadsRatio": 0, "popular": false, "version": "0.0.1-security", "description": "security holding package", "repository": { "type": "git", "url": "npm/security-holder", "project": "security-holder", "user": "npm", "host": "github.com", "path": "", "branch": "master" }, "deprecated": false, "isDeprecated": false, "homepage": null, "license": null, "keywords": [], "modified": 1745588028183, "owners": [], "objectID": "dowload_ebok_a_bela_e_a_fera_by_elizabeth_rudnick_evan_spiliotopoulos_stephen_ch_9ooey" }, { "name": "dowload_ebok_1917_une_passion_russe_by_max_gallo_d2xeh", "downloadsLast30Days": 0, "downloadsRatio": 0, "popular": false, "version": "0.0.1-security", "description": "security holding package", "repository": { "type": "git", "url": "npm/security-holder", "project": "security-holder", "user": "npm", "host": "github.com", "path": "", "branch": "master" }, "deprecated": false, "isDeprecated": false, "homepage": null, "license": null, "keywords": [], "modified": 1758520462981, "owners": [], "objectID": "dowload_ebok_1917_une_passion_russe_by_max_gallo_d2xeh" } ], "nbHits": 28, "offset": 20, "length": 8, "exhaustiveNbHits": false, "exhaustiveTypo": false, "exhaustive": { "nbHits": false, "typo": false }, "query": "download_ebook", "params": "query=download_ebook&offset=20&length=8&analyticsTags=%5B%22npmx.dev%22%5D&attributesToRetrieve=%5B%22name%22%2C%22version%22%2C%22description%22%2C%22modified%22%2C%22homepage%22%2C%22repository%22%2C%22owners%22%2C%22downloadsLast30Days%22%2C%22downloadsRatio%22%2C%22popular%22%2C%22keywords%22%2C%22deprecated%22%2C%22isDeprecated%22%2C%22license%22%5D&attributesToHighlight=%5B%5D", "index": "npm-search", "processingTimeMS": 27, "processingTimingsMS": { "_request": { "roundTrip": 5 }, "fetch": { "query": 17, "scanning": 6, "total": 24 }, "getIdx": { "load": { "total": 1 }, "total": 1 }, "total": 27 }, "serverTimeMS": 28 } ] }