-
-
Notifications
You must be signed in to change notification settings - Fork 280
perf: batch tag resolution to reduce action→query round-trips #112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add getVersionsByIds batch query to skills.ts and souls.ts - Replace per-item tag resolution with batch resolution in httpApiV1.ts - Reduces N action→query round-trips to 1 for list endpoints Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@mkrokosz is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
| export const getVersionsByIds = query({ | ||
| args: { versionIds: v.array(v.id('skillVersions')) }, | ||
| handler: async (ctx, args) => { | ||
| const versions = await Promise.all(args.versionIds.map((id) => ctx.db.get(id))) | ||
| return versions.filter((v): v is NonNullable<typeof v> => v !== null) | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] getVersionsByIds uses Promise.all(versionIds.map(ctx.db.get)), which will issue one db.get per ID anyway. This reduces action→query round-trips (good), but inside the query it can still be a large fan-out (up to all tags across the page). If versionIds can be large, consider adding a conservative cap/deduplication at the query boundary, or using an index-based fetch strategy if available.
Also appears in convex/souls.ts:148-154.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.ts
Line: 1128:1134
Comment:
[P2] `getVersionsByIds` uses `Promise.all(versionIds.map(ctx.db.get))`, which will issue one `db.get` per ID anyway. This reduces action→query round-trips (good), but inside the query it can still be a large fan-out (up to all tags across the page). If `versionIds` can be large, consider adding a conservative cap/deduplication at the query boundary, or using an index-based fetch strategy if available.
Also appears in `convex/souls.ts:148-154`.
How can I resolve this? If you propose a fix, please make it concise.- Short-circuit when no version IDs to resolve - Add null coalescing for runQuery response - Fixes potential crash when tags are empty or query returns null Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
ctx.runQuery()calls to 1Problem
The
resolveTagsfunction was called per-item in list responses, with each call making sequentialctx.runQuery()calls for each tag. For a list of 20 skills with 5 tags each, this resulted in 100 separate action→query round-trips.Solution
getVersionsByIdsbatch query toskills.tsandsouls.tsPerformance Impact
Changes
convex/skills.ts: AddgetVersionsByIdsqueryconvex/souls.ts: AddgetVersionsByIdsqueryconvex/httpApiV1.ts: ReplaceresolveTags/resolveSoulTagswith batch versionsconvex/httpApiV1.handlers.test.ts: Update tests to verify batch behaviorAPI Impact
None - response format is unchanged.
🤖 Generated with Claude Code
Greptile Overview
Greptile Summary
This PR batches version-tag resolution for skills and souls in the v1 HTTP API handlers. Instead of resolving each tag via repeated
ctx.runQuery()calls per item, handlers now collect all version IDs from the response, fetch the corresponding versions via a singlegetVersionsByIdsquery, and map the resolved version strings back onto each item.The approach fits the existing Convex pattern of keeping list endpoints cacheable/efficient while doing additional enrichment in the HTTP layer.
One correctness issue to address: the new batch resolvers assume the batch query always returns an array, and can throw when there are no tags / the query returns
null.Confidence Score: 2/5
resolveTagsBatch/resolveSoulTagsBatchassume the batch query returns an iterable array and will throw if it returnsnull(observed in tests when listing skills with no tags / whenrunQuerymock returns null). The remaining changes are straightforward and localized, and the performance intent is sound.(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
Context used:
dashboard- AGENTS.md (source)