Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions convex/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,15 @@ const skills = defineTable({
.index('by_stats_installs_all_time', ['statsInstallsAllTime', 'updatedAt'])
.index('by_batch', ['batch'])
.index('by_active_updated', ['softDeletedAt', 'updatedAt'])
.index('by_active_created', ['softDeletedAt', 'createdAt'])
.index('by_active_name', ['softDeletedAt', 'displayName'])
.index('by_active_stats_downloads', ['softDeletedAt', 'statsDownloads', 'updatedAt'])
.index('by_active_stats_stars', ['softDeletedAt', 'statsStars', 'updatedAt'])
.index('by_active_stats_installs_all_time', [
'softDeletedAt',
'statsInstallsAllTime',
'updatedAt',
])
.index('by_canonical', ['canonicalSkillId'])
.index('by_fork_of', ['forkOf.skillId'])

Expand Down
29 changes: 26 additions & 3 deletions convex/skills.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ const MAX_ACTIVE_REPORTS_PER_USER = 20
const AUTO_HIDE_REPORT_THRESHOLD = 3
const MAX_REPORT_REASON_SAMPLE = 5

const SORT_INDEXES = {
newest: 'by_active_created',
updated: 'by_active_updated',
name: 'by_active_name',
downloads: 'by_active_stats_downloads',
stars: 'by_active_stats_stars',
installs: 'by_active_stats_installs_all_time',
} as const

function isSkillVersionId(
value: Id<'skillVersions'> | null | undefined,
): value is Id<'skillVersions'> {
Expand Down Expand Up @@ -1033,14 +1042,28 @@ export const listPublicPage = query({
export const listPublicPageV2 = query({
args: {
paginationOpts: paginationOptsValidator,
sort: v.optional(
v.union(
v.literal('newest'),
v.literal('updated'),
v.literal('downloads'),
v.literal('installs'),
v.literal('stars'),
v.literal('name'),
),
),
dir: v.optional(v.union(v.literal('asc'), v.literal('desc'))),
},
handler: async (ctx, args) => {
// Use the new index to filter out soft-deleted skills at query time.
const sort = args.sort ?? 'newest'
const dir = args.dir ?? (sort === 'name' ? 'asc' : 'desc')

// Use the index to filter out soft-deleted skills at query time.
// softDeletedAt === undefined means active (non-deleted) skills only.
const result = await paginator(ctx.db, schema)
.query('skills')
.withIndex('by_active_updated', (q) => q.eq('softDeletedAt', undefined))
.order('desc')
.withIndex(SORT_INDEXES[sort], (q) => q.eq('softDeletedAt', undefined))
.order(dir)
.paginate(args.paginationOpts)
Comment on lines 1042 to 1067
Copy link

Choose a reason for hiding this comment

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

[P1] listPublicPageV2 can return a different order than the client expects when sorting by stats, because the index tie-breaker fields don’t match the client-side comparator.

For downloads/stars/installs, the index is statsX, updatedAt but the UI’s results.sort only compares the primary stat (and doesn’t fall back to updatedAt / slug). This means within equal-stat groups you’ll get a stable-but-different ordering depending on whether results came from server pagination vs the client search path.

This matters when switching between search and non-search views or when many items share the same stat value (often 0). Consider aligning the server index/order with the UI’s full comparator (e.g., add deterministic secondary keys) or updating the client comparator to match the server ordering.

Also appears in: src/routes/skills/index.tsx:169-189

Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/skills.ts
Line: 730:755

Comment:
[P1] `listPublicPageV2` can return a different order than the client expects when sorting by stats, because the index tie-breaker fields don’t match the client-side comparator.

For `downloads`/`stars`/`installs`, the index is `statsX, updatedAt` but the UI’s `results.sort` only compares the primary stat (and doesn’t fall back to `updatedAt` / slug). This means within equal-stat groups you’ll get a stable-but-different ordering depending on whether results came from server pagination vs the client search path.

This matters when switching between search and non-search views or when many items share the same stat value (often 0). Consider aligning the server index/order with the UI’s full comparator (e.g., add deterministic secondary keys) or updating the client comparator to match the server ordering.

Also appears in: `src/routes/skills/index.tsx:169-189`

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


// Build the public skill entries (fetch latestVersion + ownerHandle)
Expand Down
24 changes: 21 additions & 3 deletions convex/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,25 @@
{
"extends": "../tsconfig.json",
/* This TypeScript project config describes the environment that
* Convex functions run in and is used to typecheck them.
* You can modify it, but some settings are required to use Convex.
*/
"compilerOptions": {
/* These settings are not required by Convex and can be modified. */
"allowJs": true,
"strict": true,
"moduleResolution": "Bundler",
"skipLibCheck": true
}
"jsx": "react-jsx",
"skipLibCheck": true,
"allowSyntheticDefaultImports": true,

/* These compiler options are required by Convex */
"target": "ESNext",
"lib": ["ES2022", "dom"],
"forceConsistentCasingInFileNames": true,
"module": "ESNext",
"isolatedModules": true,
"noEmit": true
},
"include": ["./**/*"],
"exclude": ["./_generated"]
}
59 changes: 57 additions & 2 deletions src/__tests__/skills-index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ describe('SkillsIndex', () => {

it('requests the first skills page', () => {
render(<SkillsIndex />)
// usePaginatedQuery should be called with the API endpoint and empty args
// usePaginatedQuery should be called with the API endpoint and sort/dir args
expect(usePaginatedQueryMock).toHaveBeenCalledWith(
expect.anything(),
{},
{ sort: 'newest', dir: 'desc' },
{ initialNumItems: 25 },
)
})
Expand Down Expand Up @@ -118,6 +118,32 @@ describe('SkillsIndex', () => {
limit: 50,
})
})

it('sorts search results by stars and breaks ties by updatedAt', async () => {
searchMock = { q: 'remind', sort: 'stars', dir: 'desc' }
const actionFn = vi
.fn()
.mockResolvedValue([
makeSearchEntry({ slug: 'skill-a', displayName: 'Skill A', stars: 5, updatedAt: 100 }),
makeSearchEntry({ slug: 'skill-b', displayName: 'Skill B', stars: 5, updatedAt: 200 }),
makeSearchEntry({ slug: 'skill-c', displayName: 'Skill C', stars: 4, updatedAt: 999 }),
])
useActionMock.mockReturnValue(actionFn)
vi.useFakeTimers()

render(<SkillsIndex />)
await act(async () => {
await vi.runAllTimersAsync()
})
await act(async () => {
await vi.runAllTimersAsync()
})

const links = screen.getAllByRole('link')
expect(links[0]?.textContent).toContain('Skill B')
expect(links[1]?.textContent).toContain('Skill A')
expect(links[2]?.textContent).toContain('Skill C')
})
})

function makeSearchResults(count: number) {
Expand All @@ -143,3 +169,32 @@ function makeSearchResults(count: number) {
version: null,
}))
}

function makeSearchEntry(params: {
slug: string
displayName: string
stars: number
updatedAt: number
}) {
return {
score: 0.9,
skill: {
_id: `skill_${params.slug}`,
slug: params.slug,
displayName: params.displayName,
summary: `Summary ${params.slug}`,
tags: {},
stats: {
downloads: 0,
installsCurrent: 0,
installsAllTime: 0,
stars: params.stars,
versions: 1,
comments: 0,
},
createdAt: 0,
updatedAt: params.updatedAt,
},
version: null,
}
}
28 changes: 21 additions & 7 deletions src/routes/skills/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export function SkillsIndex() {
results: paginatedResults,
status: paginationStatus,
loadMore: loadMorePaginated,
} = usePaginatedQuery(api.skills.listPublicPageV2, hasQuery ? 'skip' : {}, {
} = usePaginatedQuery(api.skills.listPublicPageV2, hasQuery ? 'skip' : { sort, dir }, {
initialNumItems: pageSize,
})

Expand Down Expand Up @@ -161,32 +161,46 @@ export function SkillsIndex() {
)

const sorted = useMemo(() => {
if (!hasQuery) {
return filtered
}
const multiplier = dir === 'asc' ? 1 : -1
const results = [...filtered]
results.sort((a, b) => {
const tieBreak = () => {
const updated = (a.skill.updatedAt - b.skill.updatedAt) * multiplier
if (updated !== 0) return updated
return a.skill.slug.localeCompare(b.skill.slug)
}
switch (sort) {
case 'downloads':
return (a.skill.stats.downloads - b.skill.stats.downloads) * multiplier
return (a.skill.stats.downloads - b.skill.stats.downloads) * multiplier || tieBreak()
case 'installs':
return (
((a.skill.stats.installsAllTime ?? 0) - (b.skill.stats.installsAllTime ?? 0)) *
multiplier
multiplier || tieBreak()
)
case 'stars':
return (a.skill.stats.stars - b.skill.stats.stars) * multiplier
return (a.skill.stats.stars - b.skill.stats.stars) * multiplier || tieBreak()
case 'updated':
return (a.skill.updatedAt - b.skill.updatedAt) * multiplier
return (
(a.skill.updatedAt - b.skill.updatedAt) * multiplier ||
a.skill.slug.localeCompare(b.skill.slug)
)
case 'name':
return (
(a.skill.displayName.localeCompare(b.skill.displayName) ||
a.skill.slug.localeCompare(b.skill.slug)) * multiplier
)
default:
return (a.skill.createdAt - b.skill.createdAt) * multiplier
return (
(a.skill.createdAt - b.skill.createdAt) * multiplier ||
a.skill.slug.localeCompare(b.skill.slug)
)
}
})
return results
}, [dir, filtered, sort])
}, [dir, filtered, hasQuery, sort])

const isLoadingSkills = hasQuery ? isSearching && searchResults.length === 0 : isLoadingList
const canLoadMore = hasQuery
Expand Down