Repository navigation
Improve search discovery signals - #252
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe changes introduce guidance generation and recommended next steps for search results. A new Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Tool as Search Tool
participant Handler as searchUnified
participant Registry as Package Registry
participant Formatter as Search Formatter
Client->>Tool: search(query)
Tool->>Handler: searchUnified(query, savedPackages)
Handler->>Registry: buildSavedPackageSearchRows(records)
Registry->>Registry: buildPackageSearchProjection(manifest)
Registry-->>Handler: PackageSearchRow[]
Handler->>Handler: Match packages & connectors
Handler->>Handler: buildRecommendedNextStep(matches)
Handler-->>Tool: SearchUnifiedResult {matches, guidance}
Tool->>Formatter: formatSearchMarkdown({matches, guidance})
Formatter->>Formatter: toSlimStructuredMatches(matches)
Formatter->>Formatter: Render "## Recommended next step"
Formatter-->>Tool: Markdown output
Tool-->>Client: Formatted search results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
April 22, 2026 00:45
Contributor
|
🔎 Preview deployed: https://kody-pr-252.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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/worker/src/mcp/tools/search.ts (2)
198-218: Hardcoded alias map won't scale beyond the three seeded connectors.
wellKnownConnectorAliasesis a closed, in-code record with onlyspotify,github,tesla. Any new saved connector (slack, notion, linear, etc.) gets zero discovery boost from aliases until this map is manually extended — easy to forget, and the data belongs with connector definitions rather than search internals. Consider sourcing aliases from the connector config itself (e.g., an optionalaliases: string[]field onConnectorConfig) or a separate module co-located with connector-shared types so adding a connector is one-touch.Not blocking for this PR, but worth a follow-up issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 198 - 218, The hardcoded wellKnownConnectorAliases map and getConnectorAliases function make alias data non-extensible; instead update search logic to read aliases from connector configs (e.g., add an optional aliases: string[] to ConnectorConfig or a connector metadata module) and fall back to the existing wellKnownConnectorAliases only if no config-provided aliases exist. Specifically, modify getConnectorAliases to accept or look up the ConnectorConfig for the given connectorName (or load connector metadata) and return config.aliases when present, otherwise return wellKnownConnectorAliases[connectorName.toLowerCase()] ?? []; ensure any consumers of getConnectorAliases (search-related code) still receive an array and add unit tests for a connector with and without aliases.
705-763: Recompute ofdeterministicEmbedding(input.query)per connector row.Unlike
buildCapabilityCandidatesandbuildPackageCandidates, which accept a precomputedqueryEmbedding,buildConnectorCandidatesrecomputesdeterministicEmbedding(input.query)inside theflatMapfor every value row — even for rows that ultimately return[]becauseparseConnectorValueName/parseConnectorConfigreject them. For users with many saved values this is unnecessary work on a hot path. AcceptqueryEmbeddinglike the other builders for consistency and cost savings.♻️ Proposed refactor
function buildConnectorCandidates(input: { query: string rows: Array<ValueMetadata> + queryEmbedding: ReadonlyArray<number> }): Array<SearchCandidate> { return input.rows .flatMap((row) => { ... const lexical = lexicalScore(input.query, document) const vector = cosineSimilarity( - deterministicEmbedding(input.query), + input.queryEmbedding, deterministicEmbedding(document), )And in
searchUnified:...buildConnectorCandidates({ query: intent.normalizedQuery, rows: input.optionalRows.userValueRows, + queryEmbedding, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 705 - 763, buildConnectorCandidates recomputes deterministicEmbedding(input.query) per row which is wasteful; change buildConnectorCandidates to accept a precomputed queryEmbedding (like buildCapabilityCandidates/buildPackageCandidates) and use that for cosineSimilarity instead of calling deterministicEmbedding inside the flatMap, update the call sites (e.g., searchUnified) to compute const queryEmbedding = deterministicEmbedding(query) once and pass it into buildConnectorCandidates, and ensure the new signature (e.g., buildConnectorCandidates(input: {query: string; rows: Array<ValueMetadata>; queryEmbedding: number[]}) or similar) is used everywhere so the embedding is not recomputed for rows that are filtered out by parseConnectorValueName/parseConnectorConfig.
🤖 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/tools/search.ts`:
- Around line 153-167: The fallback projection breaks the invariant that hasApp
=== (appEntry !== null); in buildFallbackPackageSearchProjection you should
derive hasApp from the same signal as the regular projection (i.e., from
appEntry) — since this fallback forces appEntry: null, set hasApp: false to
preserve the invariant and avoid downstream mismatch with buildPackageCandidates
and buildPackageSearchDocument; if you intentionally want to keep record.hasApp
despite a null appEntry, add a clear comment explaining the divergence and
update buildPackageSearchDocument/buildPackageCandidates to handle hasApp &&
!appEntry consistently.
- Around line 169-196: buildSavedPackageSearchRows currently swallows all errors
from loadPackageSourceBySourceId; change the catch to capture and surface the
failure: in the catch block call your observability utility (e.g.
Sentry.captureException(error) or logMcpEvent with a warning outcome) and attach
a warning string for that record to the returned result instead of silently
returning only the fallback projection; update the function return shape (or
wrap results in OptionalSearchRowsResult) so callers receive both the projection
(built via buildFallbackPackageSearchProjection) and a warnings array containing
a concise message referencing the record.sourceId and error context.
- Around line 238-258: buildRecommendedNextStep currently pairs topPackage and
topConnector regardless of relation, causing misleading guidance when they are
unrelated; update the logic in buildRecommendedNextStep to only return the
combined "Found saved package ... and connector ..." guidance when the package
and connector are meaningfully related — e.g., require topPackage.kodyId or a
normalized name/alias to match topConnector.connectorName (or require
topConnector to also appear within the top N overall matches or above a
configurable score threshold on the same input) and otherwise fall back to the
single-match branches. Target the variables topPackage, topConnector and the
intent check input.intent.task.name === 'operate' when making this change.
---
Nitpick comments:
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 198-218: The hardcoded wellKnownConnectorAliases map and
getConnectorAliases function make alias data non-extensible; instead update
search logic to read aliases from connector configs (e.g., add an optional
aliases: string[] to ConnectorConfig or a connector metadata module) and fall
back to the existing wellKnownConnectorAliases only if no config-provided
aliases exist. Specifically, modify getConnectorAliases to accept or look up the
ConnectorConfig for the given connectorName (or load connector metadata) and
return config.aliases when present, otherwise return
wellKnownConnectorAliases[connectorName.toLowerCase()] ?? []; ensure any
consumers of getConnectorAliases (search-related code) still receive an array
and add unit tests for a connector with and without aliases.
- Around line 705-763: buildConnectorCandidates recomputes
deterministicEmbedding(input.query) per row which is wasteful; change
buildConnectorCandidates to accept a precomputed queryEmbedding (like
buildCapabilityCandidates/buildPackageCandidates) and use that for
cosineSimilarity instead of calling deterministicEmbedding inside the flatMap,
update the call sites (e.g., searchUnified) to compute const queryEmbedding =
deterministicEmbedding(query) once and pass it into buildConnectorCandidates,
and ensure the new signature (e.g., buildConnectorCandidates(input: {query:
string; rows: Array<ValueMetadata>; queryEmbedding: number[]}) or similar) is
used everywhere so the embedding is not recomputed for rows that are filtered
out by parseConnectorValueName/parseConnectorConfig.
🪄 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 Plus
Run ID: 5b4b138f-efb0-4540-ae3b-b27ff7abe935
📒 Files selected for processing (7)
packages/worker/src/agent-turn/tools.tspackages/worker/src/mcp/tools/search-format.node.test.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.node.test.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/package-registry/embed.tspackages/worker/src/package-registry/manifest.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 357a318. Configure here.
| source, | ||
| content: published.content, | ||
| }), | ||
| }) as LoadedPackageManifest |
Contributor
There was a problem hiding this comment.
Manifest parsed twice, ignoring already-parsed result
Medium Severity
loadPackageManifestBySourceId calls loadPublishedEntityManifest, which already parses the manifest and returns it as published.manifest. However, the caller ignores published.manifest and re-parses from published.content via parsePackageManifest. This performs redundant JSON parsing and Zod schema validation on every uncached call in the search hot path. The parsePackageManifest function in source.ts is also an exact duplicate of parsePublishedManifest in published-source.ts.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 357a318. Configure here.
This was referenced Apr 22, 2026
This was referenced May 5, 2026
This was referenced Jul 24, 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
mainand resolve thepackages/worker/src/package-registry/source.tsconflict by rebasing the search/manifest-only improvements ontomain's new published-source architectureTesting
Summary by CodeRabbit
Release Notes
New Features
Improvements