Repository navigation
Improve saved skill search discoverability - #126
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughIntroduces normalized phrase matching and per-entity lexical scoring into unified search; precomputes per-skill lexical scores; enforces a bounded candidateLimit for fetches; reworks unified ranking to combine fused RRF scores, unified lexical scores, and entity scores. Skill embed text now optionally includes a normalized skill name and a humanized variant. Changes
Sequence DiagramsequenceDiagram
participant Client
participant UnifiedSearch as Unified Search
participant SkillSearch as Skill Search
participant Embedding as Skill Embedding
participant Ranker as Multi‑Signal Ranker
Client->>UnifiedSearch: searchUnified(query, limit)
UnifiedSearch->>Ranker: normalizeSearchPhrase(query)
UnifiedSearch->>SkillSearch: fetch top candidates (candidateLimit)
SkillSearch->>Embedding: buildSkillEmbedText(skillName?, title, desc, ...)
Embedding-->>SkillSearch: embed text (includes normalized/humanized name)
SkillSearch->>Ranker: compute lexicalScoreById (lexical matcher + phrase bonuses)
Ranker->>Ranker: compute fused RRF scores (cross-entity)
Ranker->>Ranker: compute unified lexical score per entity type
Ranker-->>UnifiedSearch: ranked skill results (by fused → lexical → vector)
UnifiedSearch->>Ranker: unify results from skills/capabilities/secrets/ui_artifacts
Ranker->>Ranker: final sort by fused RRF → unified lexical → entity score → key
Ranker-->>UnifiedSearch: unified ranked list
UnifiedSearch-->>Client: return final results (bounded by original limit)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
kentcdodds
marked this pull request as ready for review
March 31, 2026 23:57
Contributor
|
🔎 Preview deployed: https://kody-pr-126.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/worker/src/mcp/capabilities/unified-search.ts (1)
548-584: Consider caching the normalized query to avoid repeated computation.
normalizeSearchPhrase(input.query)is called multiple times per skill hit withingetUnifiedLexicalScore(lines 564, 566, 568-570). This could be computed once and passed as a parameter or cached at the outer scope.🔧 Cache normalized query at outer scope
+ const normalizedQuery = normalizeSearchPhrase(input.query) function getUnifiedLexicalScore(key: string): number { if (key.startsWith('c:')) { const hit = capByName.get(key.slice(2)) return hit ? scoreCapabilityLexicalMatch(input.query, hit) : 0 } if (key.startsWith('s:')) { const hit = skillByName.get(key.slice(2)) if (!hit) return 0 return ( lexicalScore(input.query, [ hit.skillName, hit.title, hit.description, hit.collection ?? '', hit.keywords.join(' '), ].join('\n')) + - scoreSkillPhraseMatch(normalizeSearchPhrase(input.query), hit.skillName) * + scoreSkillPhraseMatch(normalizedQuery, hit.skillName) * 2 + - scoreSkillPhraseMatch(normalizeSearchPhrase(input.query), hit.title) * + scoreSkillPhraseMatch(normalizedQuery, hit.title) * 1.5 + scoreSkillPhraseMatch( - normalizeSearchPhrase(input.query), + normalizedQuery, hit.description, ) * 1.25 ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/unified-search.ts` around lines 548 - 584, In getUnifiedLexicalScore compute normalizeSearchPhrase(input.query) once and reuse it instead of calling normalizeSearchPhrase(input.query) multiple times; e.g., create a local const (e.g., normalizedQuery) in the scope where getUnifiedLexicalScore can access input.query (or at the start of getUnifiedLexicalScore) and pass that to scoreSkillPhraseMatch calls for hit.skillName, hit.title, and hit.description, leaving lexicalScore(...) as-is to avoid repeated normalization work.packages/worker/src/mcp/skills/skill-embed-and-flags.ts (1)
42-50: Redundant inner conditional forhumanizedSkillName.When
normalizedSkillNameis truthy (line 45 condition is met),humanizedSkillNamewill always be a non-empty string since it's derived fromnormalizedSkillNamevia.replace(/[-_]+/g, ' '). The inner conditional on line 48 is always true in this context.🔧 Simplify by removing redundant conditional
const baseParts = [ ...(normalizedSkillName ? [ `name ${normalizedSkillName}`, - ...(humanizedSkillName ? [humanizedSkillName] : []), + humanizedSkillName, ] : []),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/skills/skill-embed-and-flags.ts` around lines 42 - 50, The inner conditional checking humanizedSkillName inside the baseParts spread is redundant because humanizedSkillName is derived from normalizedSkillName and will be non-empty whenever normalizedSkillName is truthy; update the baseParts construction (around normalizedSkillName, humanizedSkillName, baseParts) to always include humanizedSkillName in the array when normalizedSkillName is present instead of the nested conditional, simplifying the branch and removing the unnecessary check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/unified-search.ts`:
- Around line 548-584: In getUnifiedLexicalScore compute
normalizeSearchPhrase(input.query) once and reuse it instead of calling
normalizeSearchPhrase(input.query) multiple times; e.g., create a local const
(e.g., normalizedQuery) in the scope where getUnifiedLexicalScore can access
input.query (or at the start of getUnifiedLexicalScore) and pass that to
scoreSkillPhraseMatch calls for hit.skillName, hit.title, and hit.description,
leaving lexicalScore(...) as-is to avoid repeated normalization work.
In `@packages/worker/src/mcp/skills/skill-embed-and-flags.ts`:
- Around line 42-50: The inner conditional checking humanizedSkillName inside
the baseParts spread is redundant because humanizedSkillName is derived from
normalizedSkillName and will be non-empty whenever normalizedSkillName is
truthy; update the baseParts construction (around normalizedSkillName,
humanizedSkillName, baseParts) to always include humanizedSkillName in the array
when normalizedSkillName is present instead of the nested conditional,
simplifying the branch and removing the unnecessary check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ad0f958-713b-495b-bc9a-648d6207b0a7
📒 Files selected for processing (6)
packages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/capabilities/unified-search.workers.test.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/mcp/skills/skill-embed-and-flags.node.test.tspackages/worker/src/mcp/skills/skill-embed-and-flags.tspackages/worker/src/mcp/skills/skill-mutation.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Lexical scores recomputed inside sort comparator instead of precomputed
- Cached unified lexical scores in a map before sorting to avoid repeated expensive recomputation during comparisons.
You can send follow-ups to this agent here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/unified-search.ts (1)
548-584: Consider extracting shared lexical scoring logic to reduce duplication.The skill scoring logic in
getUnifiedLexicalScore(lines 553-573) duplicates the weighted phrase-match pattern fromscoreSkillLexicalMatch. Since they operate on different types (SkillSearchHitvsMcpSkillRow), some duplication is inevitable, but extracting a shared helper that accepts the relevant fields could improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/unified-search.ts` around lines 548 - 584, getUnifiedLexicalScore duplicates the weighted phrase-match logic used in scoreSkillLexicalMatch for skills; extract a small shared helper (e.g., buildSkillTextScore or scoreSkillLike) that takes the search text (input.query) and a normalized/adapter object or explicit fields (skillName, title, description, collection, keywords) so both getUnifiedLexicalScore and scoreSkillLexicalMatch can call it; use existing helpers lexicalScore, scoreSkillPhraseMatch, and normalizeSearchPhrase inside the new helper and update getUnifiedLexicalScore to call it when handling 's:' keys (operating on SkillSearchHit) and update scoreSkillLexicalMatch to call it for McpSkillRow, mapping fields as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/capabilities/unified-search.ts`:
- Around line 105-118: The doc string construction in
scoreUiArtifactLexicalMatch can include literal "undefined" when hit.runtime,
hit.description, or parameter.description are missing; update the joins to skip
falsy parts (e.g., filter(Boolean)) so only defined strings are concatenated and
also filter/map parameterText entries (or default missing parameter.description
to '') before joining; keep the existing behavior of scoring but ensure the
array passed to join for doc and parameterText excludes undefined/null values to
avoid injecting "undefined" into the document used by lexicalScore.
- Around line 84-94: In scoreCapabilityLexicalMatch, the doc string is built by
joining [hit.name, hit.domain, hit.description] which can introduce the literal
"undefined" when hit.domain or hit.description are undefined; update the
construction of doc inside scoreCapabilityLexicalMatch to only include
defined/nonnull fields (e.g., build an array [hit.name, hit.domain,
hit.description] and filter out undefined/null/empty values before joining with
'\n') so lexicalScore receives a clean document string.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/unified-search.ts`:
- Around line 548-584: getUnifiedLexicalScore duplicates the weighted
phrase-match logic used in scoreSkillLexicalMatch for skills; extract a small
shared helper (e.g., buildSkillTextScore or scoreSkillLike) that takes the
search text (input.query) and a normalized/adapter object or explicit fields
(skillName, title, description, collection, keywords) so both
getUnifiedLexicalScore and scoreSkillLexicalMatch can call it; use existing
helpers lexicalScore, scoreSkillPhraseMatch, and normalizeSearchPhrase inside
the new helper and update getUnifiedLexicalScore to call it when handling 's:'
keys (operating on SkillSearchHit) and update scoreSkillLexicalMatch to call it
for McpSkillRow, mapping fields as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e2951c7-91d4-40f5-921b-1dfdf98af7b4
📒 Files selected for processing (1)
packages/worker/src/mcp/capabilities/unified-search.ts
Comment on lines
+84
to
+94
| function scoreCapabilityLexicalMatch( | ||
| query: string, | ||
| hit: CapabilitySearchHit, | ||
| ): number { | ||
| const normalizedQuery = normalizeSearchPhrase(query) | ||
| const doc = [hit.name, hit.domain, hit.description].join('\n') | ||
| let bonus = 0 | ||
| bonus += scoreSkillPhraseMatch(normalizedQuery, hit.name) * 1.5 | ||
| bonus += scoreSkillPhraseMatch(normalizedQuery, hit.description) * 1 | ||
| return lexicalScore(query, doc) + bonus | ||
| } |
There was a problem hiding this comment.
Filter out undefined values before joining to avoid "undefined" string in doc.
If hit.domain or hit.description is undefined, Array.join will convert it to the literal string "undefined", which could affect lexical matching.
Proposed fix
function scoreCapabilityLexicalMatch(
query: string,
hit: CapabilitySearchHit,
): number {
const normalizedQuery = normalizeSearchPhrase(query)
- const doc = [hit.name, hit.domain, hit.description].join('\n')
+ const doc = [hit.name, hit.domain, hit.description].filter(Boolean).join('\n')
let bonus = 0
bonus += scoreSkillPhraseMatch(normalizedQuery, hit.name) * 1.5
bonus += scoreSkillPhraseMatch(normalizedQuery, hit.description) * 1
return lexicalScore(query, doc) + bonus
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/unified-search.ts` around lines 84 - 94,
In scoreCapabilityLexicalMatch, the doc string is built by joining [hit.name,
hit.domain, hit.description] which can introduce the literal "undefined" when
hit.domain or hit.description are undefined; update the construction of doc
inside scoreCapabilityLexicalMatch to only include defined/nonnull fields (e.g.,
build an array [hit.name, hit.domain, hit.description] and filter out
undefined/null/empty values before joining with '\n') so lexicalScore receives a
clean document string.
Comment on lines
+105
to
+118
| function scoreUiArtifactLexicalMatch( | ||
| query: string, | ||
| hit: UiArtifactSearchHit, | ||
| ): number { | ||
| const normalizedQuery = normalizeSearchPhrase(query) | ||
| const parameterText = (hit.parameters ?? []) | ||
| .map((parameter) => `${parameter.name} ${parameter.description}`) | ||
| .join('\n') | ||
| const doc = [hit.title, hit.description, hit.runtime, parameterText].join('\n') | ||
| let bonus = 0 | ||
| bonus += scoreSkillPhraseMatch(normalizedQuery, hit.title) * 1.5 | ||
| bonus += scoreSkillPhraseMatch(normalizedQuery, hit.description) * 1 | ||
| return lexicalScore(query, doc) + bonus | ||
| } |
There was a problem hiding this comment.
Same filter(Boolean) recommendation applies here.
hit.runtime or hit.description being undefined would inject literal "undefined" into the doc string.
Proposed fix
function scoreUiArtifactLexicalMatch(
query: string,
hit: UiArtifactSearchHit,
): number {
const normalizedQuery = normalizeSearchPhrase(query)
const parameterText = (hit.parameters ?? [])
.map((parameter) => `${parameter.name} ${parameter.description}`)
.join('\n')
- const doc = [hit.title, hit.description, hit.runtime, parameterText].join('\n')
+ const doc = [hit.title, hit.description, hit.runtime, parameterText].filter(Boolean).join('\n')
let bonus = 0
bonus += scoreSkillPhraseMatch(normalizedQuery, hit.title) * 1.5
bonus += scoreSkillPhraseMatch(normalizedQuery, hit.description) * 1
return lexicalScore(query, doc) + bonus
}📝 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.
Suggested change
| function scoreUiArtifactLexicalMatch( | |
| query: string, | |
| hit: UiArtifactSearchHit, | |
| ): number { | |
| const normalizedQuery = normalizeSearchPhrase(query) | |
| const parameterText = (hit.parameters ?? []) | |
| .map((parameter) => `${parameter.name} ${parameter.description}`) | |
| .join('\n') | |
| const doc = [hit.title, hit.description, hit.runtime, parameterText].join('\n') | |
| let bonus = 0 | |
| bonus += scoreSkillPhraseMatch(normalizedQuery, hit.title) * 1.5 | |
| bonus += scoreSkillPhraseMatch(normalizedQuery, hit.description) * 1 | |
| return lexicalScore(query, doc) + bonus | |
| } | |
| function scoreUiArtifactLexicalMatch( | |
| query: string, | |
| hit: UiArtifactSearchHit, | |
| ): number { | |
| const normalizedQuery = normalizeSearchPhrase(query) | |
| const parameterText = (hit.parameters ?? []) | |
| .map((parameter) => `${parameter.name} ${parameter.description}`) | |
| .join('\n') | |
| const doc = [hit.title, hit.description, hit.runtime, parameterText].filter(Boolean).join('\n') | |
| let bonus = 0 | |
| bonus += scoreSkillPhraseMatch(normalizedQuery, hit.title) * 1.5 | |
| bonus += scoreSkillPhraseMatch(normalizedQuery, hit.description) * 1 | |
| return lexicalScore(query, doc) + bonus | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/unified-search.ts` around lines 105 -
118, The doc string construction in scoreUiArtifactLexicalMatch can include
literal "undefined" when hit.runtime, hit.description, or parameter.description
are missing; update the joins to skip falsy parts (e.g., filter(Boolean)) so
only defined strings are concatenated and also filter/map parameterText entries
(or default missing parameter.description to '') before joining; keep the
existing behavior of scoring but ensure the array passed to join for doc and
parameterText excludes undefined/null values to avoid injecting "undefined" into
the document used by lexicalScore.
This was referenced Apr 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
launch-cursor-cloud-agentdiscoverability withoutskill_collectionTesting
npm run test -- packages/worker/src/mcp/skills/skill-embed-and-flags.node.test.ts packages/worker/src/mcp/capabilities/unified-search.workers.test.tsnpm run test:mcp -- packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts/opt/cursor/artifacts/search-ranking-validation.txtSummary by CodeRabbit
Improvements
Tests