perf: denormalize latestVersionSummary into skillSearchDigest#776
perf: denormalize latestVersionSummary into skillSearchDigest#776sethconvex wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Eliminates ~9MB of skillVersions reads per listPublicPageV2 call by copying latestVersionSummary from skills into the digest via the existing trigger. Old rows without the field fall back to fetching the full version doc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Someone is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR denormalizes
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: convex/skills.listPublicPageV2.test.ts
Line: 336-341
Comment:
**Fallback path no longer covered by tests**
`makeSkill` now unconditionally includes `latestVersionSummary`, so every test in this file exercises the "use summary" fast path. The regression path — where `latestVersionSummary` is `undefined` and `buildPublicSkillEntries` must fall back to `ctx.db.get(skill.latestVersionId)` — is no longer tested. This matters because the PR description explicitly calls out that backfilling existing digest rows is a post-deploy step, meaning old rows without the field will hit this code path in production.
Consider adding a test with `latestVersionSummary: undefined` (or omitting it) that verifies `db.get` is called with the version ID and the result is surfaced correctly.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 576588b |
| latestVersionSummary: { | ||
| version: '1.0.0', | ||
| createdAt: 1, | ||
| changelog: '', | ||
| changelogSource: 'user' as const, | ||
| }, |
There was a problem hiding this comment.
Fallback path no longer covered by tests
makeSkill now unconditionally includes latestVersionSummary, so every test in this file exercises the "use summary" fast path. The regression path — where latestVersionSummary is undefined and buildPublicSkillEntries must fall back to ctx.db.get(skill.latestVersionId) — is no longer tested. This matters because the PR description explicitly calls out that backfilling existing digest rows is a post-deploy step, meaning old rows without the field will hit this code path in production.
Consider adding a test with latestVersionSummary: undefined (or omitting it) that verifies db.get is called with the version ID and the result is surfaced correctly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.listPublicPageV2.test.ts
Line: 336-341
Comment:
**Fallback path no longer covered by tests**
`makeSkill` now unconditionally includes `latestVersionSummary`, so every test in this file exercises the "use summary" fast path. The regression path — where `latestVersionSummary` is `undefined` and `buildPublicSkillEntries` must fall back to `ctx.db.get(skill.latestVersionId)` — is no longer tested. This matters because the PR description explicitly calls out that backfilling existing digest rows is a post-deploy step, meaning old rows without the field will hit this code path in production.
Consider adding a test with `latestVersionSummary: undefined` (or omitting it) that verifies `db.get` is called with the version ID and the result is surfaced correctly.
How can I resolve this? If you propose a fix, please make it concise.Verifies that old digest rows without latestVersionSummary correctly fall back to ctx.db.get(latestVersionId) for version data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b32a499774
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| 'latestVersionSummary' in skill | ||
| ? (skill as Doc<'skills'>).latestVersionSummary | ||
| : undefined | ||
| const summary = skill.latestVersionSummary |
There was a problem hiding this comment.
Add a migration to populate digest latestVersionSummary
Switching listPublicPageV2 to trust skill.latestVersionSummary here means the optimization only works if old skillSearchDigest rows are populated, but this commit never backfills that table: the sync path only runs when a skills row mutates (convex/functions.ts trigger), and backfillLatestVersionSummaryInternal skips already-synced skill rows (convex/maintenance.ts), so most existing digests will keep the field absent and continue hitting ctx.db.get(skill.latestVersionId) on every page. In production this prevents the intended read reduction for historical data; add a one-time digest backfill (or force-touch skills) as part of this change.
Useful? React with 👍 / 👎.
Summary
latestVersionSummarytoskillSearchDigestschema and syncs it via the existing trigger (SHARED_KEYS)buildPublicSkillEntriesnow reads summary directly fromHydratableSkillinstead of type-narrowing with'latestVersionSummary' in skillskillVersionsreads perlistPublicPageV2call; old rows without the field fall back to fetching the full version docTest plan
npx vitest run— all 714 tests passnpx convex dev --once— typecheck + push to devbackfillLatestVersionSummaryto populate existing digest rowsskillVersionsshould disappear fromlistPublicPageV2🤖 Generated with Claude Code