Skip to content

fix(api): restore v1 skills list compatibility #961

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

Closed
Alix-007 wants to merge 4 commits into from
Closed

fix(api): restore v1 skills list compatibility #961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
20b81bf
fix(api): restore v1 skills list compatibility
Alix-007 Mar 17, 2026
30a4e76
test(api): cover v1 skills list compatibility shim
Alix-007 Mar 17, 2026
ab32fce
fix(api): add trending leaderboard fallback
Alix-007 Mar 17, 2026
d71ca17
test(api): cover stat sorts and trending fallback
Alix-007 Mar 17, 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
373 changes: 361 additions & 12 deletions convex/skills.listPublicPage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type ListArgs = {
}

type ListResult = {
items: unknown[]
items: Array<{ skill: { slug: string } }>
nextCursor: string | null
}

Expand All @@ -27,25 +27,374 @@ type WrappedHandler<TArgs, TResult> = {
const listPublicPageHandler = (listPublicPage as unknown as WrappedHandler<ListArgs, ListResult>)
._handler

describe('skills.listPublicPage (deprecated stub)', () => {
it('returns empty results with no DB reads', async () => {
const ctx = {
db: {
query: vi.fn(),
get: vi.fn(),
normalizeId: vi.fn(),
},
}
describe('skills.listPublicPage', () => {
it('filters suspicious skills when nonSuspiciousOnly is enabled', async () => {
const clean = makeSkill('skills:clean', 'clean', 'users:1', 'skillVersions:1')
const suspicious = makeSkill(
'skills:suspicious',
'suspicious',
'users:2',
'skillVersions:2',
['flagged.suspicious'],
)

const paginateMock = vi.fn().mockResolvedValue({
page: [clean, suspicious],
continueCursor: 'next',
isDone: false,
})
const ctx = makeCtx({
by_updated: paginateMock,
users: [makeUser('users:1'), makeUser('users:2')],
versions: [makeVersion('skillVersions:1'), makeVersion('skillVersions:2')],
})

const result = await listPublicPageHandler(ctx, {
sort: 'updated',
limit: 10,
nonSuspiciousOnly: true,
})

expect(result.items).toHaveLength(1)
expect(result.items[0]?.skill.slug).toBe('clean')
expect(result.nextCursor).toBe('next')
})

it('returns suspicious skills when nonSuspiciousOnly is disabled', async () => {
const clean = makeSkill('skills:clean', 'clean', 'users:1', 'skillVersions:1')
const suspicious = makeSkill(
'skills:suspicious',
'suspicious',
'users:2',
'skillVersions:2',
['flagged.suspicious'],
)

const paginateMock = vi.fn().mockResolvedValue({
page: [clean, suspicious],
continueCursor: null,
isDone: true,
})
const ctx = makeCtx({
by_updated: paginateMock,
users: [makeUser('users:1'), makeUser('users:2')],
versions: [makeVersion('skillVersions:1'), makeVersion('skillVersions:2')],
})

const result = await listPublicPageHandler(ctx, {
sort: 'updated',
limit: 10,
nonSuspiciousOnly: false,
})

expect(result.items).toHaveLength(2)
expect(result.items.map((entry) => entry.skill.slug)).toEqual(['clean', 'suspicious'])
})

it.each([
['downloads', 'by_stats_downloads'],
['stars', 'by_stats_stars'],
['installsCurrent', 'by_stats_installs_current'],
['installsAllTime', 'by_stats_installs_all_time'],
] as const)('uses %s compatibility index', async (sort, index) => {
const clean = makeSkill('skills:clean', 'clean', 'users:1', 'skillVersions:1')
const suspicious = makeSkill(
'skills:suspicious',
'suspicious',
'users:2',
'skillVersions:2',
['flagged.suspicious'],
)

const paginateMock = vi.fn().mockResolvedValue({
page: [clean, suspicious],
continueCursor: 'next',
isDone: false,
})
const ctx = makeCtx({
by_updated: vi.fn(),
by_index: { [index]: paginateMock },
users: [makeUser('users:1'), makeUser('users:2')],
versions: [makeVersion('skillVersions:1'), makeVersion('skillVersions:2')],
})

const result = await listPublicPageHandler(ctx, {
sort,
limit: 10,
nonSuspiciousOnly: true,
})

expect(result.items).toHaveLength(1)
expect(result.items[0]?.skill.slug).toBe('clean')
expect(result.nextCursor).toBe('next')
expect(paginateMock).toHaveBeenCalledWith({ cursor: null, numItems: 10 })
})

it('backfills clean trending skills when nonSuspiciousOnly is enabled', async () => {
const suspicious1 = makeSkill(
'skills:suspicious1',
'suspicious-1',
'users:1',
'skillVersions:1',
['flagged.suspicious'],
)
const suspicious2 = makeSkill(
'skills:suspicious2',
'suspicious-2',
'users:2',
'skillVersions:2',
['flagged.suspicious'],
)
const clean = makeSkill('skills:clean', 'clean', 'users:3', 'skillVersions:3')

const ctx = makeTrendingCtx({
leaderboards: {
trending: [suspicious1._id, suspicious2._id],
trending_non_suspicious: [clean._id],
},
skills: [suspicious1, suspicious2, clean],
users: [makeUser('users:1'), makeUser('users:2'), makeUser('users:3')],
versions: [
makeVersion('skillVersions:1'),
makeVersion('skillVersions:2'),
makeVersion('skillVersions:3'),
],
})

const result = await listPublicPageHandler(ctx, {
sort: 'trending',
limit: 1,
nonSuspiciousOnly: true,
})

expect(result.items).toHaveLength(1)
expect(result.items[0]?.skill.slug).toBe('clean')
expect(result.nextCursor).toBeNull()
})

it('falls back to the general trending leaderboard when the non-suspicious cache is missing', async () => {
const suspicious = makeSkill(
'skills:suspicious',
'suspicious',
'users:1',
'skillVersions:1',
['flagged.suspicious'],
)
const clean = makeSkill('skills:clean', 'clean', 'users:2', 'skillVersions:2')

const ctx = makeTrendingCtx({
leaderboards: {
trending: [suspicious._id, clean._id],
},
skills: [suspicious, clean],
users: [makeUser('users:1'), makeUser('users:2')],
versions: [makeVersion('skillVersions:1'), makeVersion('skillVersions:2')],
})

const result = await listPublicPageHandler(ctx, {
sort: 'trending',
limit: 1,
nonSuspiciousOnly: true,
})

expect(result.items).toHaveLength(1)
expect(result.items[0]?.skill.slug).toBe('clean')
expect(result.nextCursor).toBeNull()
})

it('returns an empty trending page when no cached leaderboard exists yet', async () => {
const ctx = makeTrendingCtx({
leaderboards: {},
skills: [],
users: [],
versions: [],
})

const result = await listPublicPageHandler(ctx, {
sort: 'trending',
limit: 10,
nonSuspiciousOnly: false,
})

expect(result.items).toEqual([])
expect(result.nextCursor).toBeNull()
expect(ctx.db.query).not.toHaveBeenCalled()
expect(ctx.db.get).not.toHaveBeenCalled()
})
})

function makeCtx({
by_updated,
by_index,
users,
versions,
}: {
by_updated: ReturnType<typeof vi.fn>
by_index?: Record<string, ReturnType<typeof vi.fn>>
users: Array<ReturnType<typeof makeUser>>
versions: Array<ReturnType<typeof makeVersion>>
}) {
const userMap = new Map(users.map((user) => [user._id, user]))
const versionMap = new Map(versions.map((version) => [version._id, version]))
return {
db: {
query: vi.fn((table: string) => {
if (table !== 'skills') throw new Error(`unexpected table ${table}`)
return {
withIndex: vi.fn((index: string, _builder: unknown) => {
const paginate = index === 'by_updated' ? by_updated : by_index?.[index]
if (!paginate) throw new Error(`unexpected index ${index}`)
return {
order: vi.fn((dir: string) => {
if (dir !== 'desc') throw new Error(`unexpected order ${dir}`)
return { paginate }
}),
}
}),
}
}),
get: vi.fn(async (id: string) => {
if (id.startsWith('users:')) return userMap.get(id) ?? null
if (id.startsWith('skillVersions:')) return versionMap.get(id) ?? null
return null
}),
},
}
Comment on lines +224 to +260
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

P2 No test coverage for stat-sorted paths

makeCtx only wires up by_updated — calling the handler with sort: 'downloads', 'stars', 'installsCurrent', or 'installsAllTime' would throw unexpected index from the mock. The four sortToIndex branches and their pagination / filtering logic have zero test coverage in this suite.

Consider adding at least one parameterised case for a stat-sorted path (e.g. sort: 'downloads') to guard against a wrong index name or a logic divergence from the updated path.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: convex/skills.listPublicPage.test.ts
Line: 155-188

Comment:
**No test coverage for stat-sorted paths**

`makeCtx` only wires up `by_updated` — calling the handler with `sort: 'downloads'`, `'stars'`, `'installsCurrent'`, or `'installsAllTime'` would throw `unexpected index` from the mock. The four `sortToIndex` branches and their pagination / filtering logic have zero test coverage in this suite.

Consider adding at least one parameterised case for a stat-sorted path (e.g. `sort: 'downloads'`) to guard against a wrong index name or a logic divergence from the `updated` path.

How can I resolve this? If you propose a fix, please make it concise.

}

function makeTrendingCtx({
leaderboards,
skills,
users,
versions,
}: {
leaderboards: Record<string, string[]>
skills: Array<ReturnType<typeof makeSkill>>
users: Array<ReturnType<typeof makeUser>>
versions: Array<ReturnType<typeof makeVersion>>
}) {
const skillMap = new Map(skills.map((skill) => [skill._id, skill]))
const userMap = new Map(users.map((user) => [user._id, user]))
const versionMap = new Map(versions.map((version) => [version._id, version]))

return {
db: {
query: vi.fn((table: string) => {
if (table !== 'skillLeaderboards') throw new Error(`unexpected table ${table}`)
return {
withIndex: vi.fn(
(
index: string,
builder: (q: { eq: (field: string, value: string) => unknown }) => unknown,
) => {
if (index !== 'by_kind') throw new Error(`unexpected index ${index}`)
let requestedKind = 'trending'
builder({
eq: (field: string, value: string) => {
if (field !== 'kind') throw new Error(`unexpected field ${field}`)
requestedKind = value
return {}
},
})
return {
order: vi.fn((dir: string) => {
if (dir !== 'desc') throw new Error(`unexpected order ${dir}`)
return {
take: vi.fn().mockResolvedValue(
leaderboards[requestedKind] !== undefined
? [
{
kind: requestedKind,
generatedAt: 1,
rangeStartDay: 1,
rangeEndDay: 1,
items: leaderboards[requestedKind].map((skillId, idx) => ({
skillId,
score: 100 - idx,
installs: 10 - idx,
downloads: 20 - idx,
})),
},
]
: [],
),
}
}),
}
},
),
}
}),
get: vi.fn(async (id: string) => {
if (id.startsWith('skills:')) return skillMap.get(id) ?? null
if (id.startsWith('users:')) return userMap.get(id) ?? null
if (id.startsWith('skillVersions:')) return versionMap.get(id) ?? null
return null
}),
},
}
}

function makeSkill(
id: string,
slug: string,
ownerUserId: string,
latestVersionId: string,
moderationFlags?: string[],
) {
return {
_id: id,
_creationTime: 1,
slug,
displayName: slug,
summary: `${slug} summary`,
ownerUserId,
canonicalSkillId: undefined,
forkOf: undefined,
latestVersionId,
tags: {},
badges: {},
statsDownloads: 0,
statsStars: 0,
statsInstallsCurrent: 0,
statsInstallsAllTime: 0,
stats: {
downloads: 0,
stars: 0,
installsCurrent: 0,
installsAllTime: 0,
versions: 1,
comments: 0,
},
moderationStatus: 'active',
moderationReason: undefined,
moderationFlags,
softDeletedAt: undefined,
createdAt: 1,
updatedAt: 1,
}
}

function makeUser(id: string) {
return {
_id: id,
_creationTime: 1,
handle: `h-${id}`,
name: 'Owner',
displayName: 'Owner',
image: null,
bio: null,
deletedAt: undefined,
deactivatedAt: undefined,
}
}

function makeVersion(id: string) {
return {
_id: id,
_creationTime: 1,
version: '1.0.0',
createdAt: 1,
changelog: '',
changelogSource: 'user',
parsed: {},
}
}
Loading