Skip to content

perf: denormalize latestVersionSummary into skillSearchDigest #776

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.

Sign up for GitHub

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

Merged
magicseth merged 2 commits into from
Mar 13, 2026
Merged

perf: denormalize latestVersionSummary into skillSearchDigest #776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
576588b
perf: denormalize latestVersionSummary into skillSearchDigest
Mar 12, 2026
b32a499
test: add coverage for fallback when latestVersionSummary is absent
Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions convex/lib/public.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export type HydratableSkill = Pick<
| 'canonicalSkillId'
| 'forkOf'
| 'latestVersionId'
| 'latestVersionSummary'
| 'tags'
| 'badges'
| 'stats'
Expand Down
2 changes: 1 addition & 1 deletion convex/lib/skillSearchDigest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ describe('extractDigestFields', () => {

expect(digest).not.toHaveProperty('moderationEvidence')
expect(digest).not.toHaveProperty('quality')
expect(digest).not.toHaveProperty('latestVersionSummary')
expect(digest).toHaveProperty('latestVersionSummary')
expect(digest).not.toHaveProperty('moderationNotes')
expect(digest).not.toHaveProperty('moderationSummary')
expect(digest).not.toHaveProperty('resourceId')
Expand Down
1 change: 1 addition & 0 deletions convex/lib/skillSearchDigest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const SHARED_KEYS = [
'canonicalSkillId',
'forkOf',
'latestVersionId',
'latestVersionSummary',
'tags',
'badges',
'stats',
Expand Down
11 changes: 11 additions & 0 deletions convex/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,17 @@ const skillSearchDigest = defineTable({
canonicalSkillId: v.optional(v.id('skills')),
forkOf: forkOfValidator,
latestVersionId: v.optional(v.id('skillVersions')),
latestVersionSummary: v.optional(
v.object({
version: v.string(),
createdAt: v.number(),
changelog: v.string(),
changelogSource: v.optional(
v.union(v.literal('auto'), v.literal('user')),
),
clawdis: v.optional(v.any()),
}),
),
tags: v.record(v.string(), v.id('skillVersions')),
badges: badgesValidator,
stats: statsValidator,
Expand Down
48 changes: 48 additions & 0 deletions convex/skills.listPublicPageV2.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,48 @@ describe('skills.listPublicPageV2', () => {
)
})

it('falls back to db.get(latestVersionId) when latestVersionSummary is absent', async () => {
const oldRow = makeSkill('skills:old', 'old', 'users:1', 'skillVersions:1')
// Simulate a pre-backfill digest row without latestVersionSummary
delete (oldRow as Record<string, unknown>).latestVersionSummary

const paginateMock = vi.fn().mockResolvedValue({
page: [oldRow],
continueCursor: 'next-cursor',
isDone: false,
pageStatus: null,
splitCursor: null,
})
const getMock = vi.fn(async (id: string) => {
if (id.startsWith('users:')) return makeUser(id)
if (id.startsWith('skillVersions:')) return makeVersion(id)
return null
})
const ctx = {
db: {
query: vi.fn(() => ({
withIndex: vi.fn(() => ({
order: vi.fn(() => ({ paginate: paginateMock })),
})),
})),
get: getMock,
},
}

const result = await listPublicPageV2Handler(ctx, {
paginationOpts: { cursor: null, numItems: 25 },
sort: 'downloads',
dir: 'desc',
highlightedOnly: false,
nonSuspiciousOnly: false,
})

expect(result.page).toHaveLength(1)
expect(result.page[0]?.skill.slug).toBe('old')
// Should have fetched the version doc via db.get
expect(getMock).toHaveBeenCalledWith('skillVersions:1')
})

it('does not swallow non-cursor paginate errors', async () => {
const paginateMock = vi.fn().mockRejectedValue(new Error('database unavailable'))
const ctx = {
Expand Down Expand Up @@ -333,6 +375,12 @@ function makeSkill(
canonicalSkillId: undefined,
forkOf: undefined,
latestVersionId,
latestVersionSummary: {
version: '1.0.0',
createdAt: 1,
changelog: '',
changelogSource: 'user' as const,
},
Comment on lines +378 to +383
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tags: {},
badges: {},
stats: {
Expand Down
6 changes: 1 addition & 5 deletions convex/skills.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,11 +958,7 @@ async function buildPublicSkillEntries(
const entries = await Promise.all(
skills.map(async (skill) => {
// Use denormalized summary when available to avoid reading the full ~6KB version doc.
// HydratableSkill (from digest rows) won't have latestVersionSummary.
const summary =
'latestVersionSummary' in skill
? (skill as Doc<'skills'>).latestVersionSummary
: undefined
const summary = skill.latestVersionSummary
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

const hasSummary = includeVersion && summary
const [latestVersionDoc, ownerInfo] = await Promise.all([
includeVersion && !hasSummary && skill.latestVersionId
Expand Down
Loading